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: HybridAppScreen.Recording.2026-08-25.at.9.30.39.AM.mp4Android: mWeb ChromeScreen_Recording_20260825_094413_Chrome.mp4iOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-08-25.at.8.01.54.AM.mov |
|
@tgolen, could you please check this. it still seems to duplicate rows. RF.mp4 |
Reverting externalID from ADVANCED_CSV_COLUMNS stopped a Unique-ID-only feed from re-importing in advanced mode, which left the role off the dropdown entirely: re-uploading the same file from the feed's settings could no longer map Unique ID, so every row duplicated. Offer the role whenever the saved layout maps it, while still never applying the saved index. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@marufsharifi Thank you for catching that. You are right. I realize that the test steps were incorrect. You're seeing duplicates because you've uploaded the file as a second feed, and the expenses are only de-duped in a single feed (not across different feeds). I've updated the test steps (from |
|
@tgolen, could you please merge the main. thanks. |
|
@marufsharifi Done! |
|
It shows that 3 new transactions were added in the modal instead of 0. This is the second time I’ve imported the same file with the same 3 rows. Is this expected behavior? Since these transactions are duplicates, I assume they shouldn’t be added again. RF-1.mp4 |
|
I have two files: one with 3 rows and another with 4 rows. The second file contains the same 3 rows from the first file, plus 1 new row. When I import the first file, 3 transactions are added as expected. However, when I import the second file, the new transaction is not added, even though the other 3 rows are duplicates. Could you please take a look and check why the new transaction isn’t being imported? Thanks. RF-2.mp4 |
Yes, this is expected. The de-duplication happens on the backend, so it's not really possible for that message to be duplicate aware. I think that's OK for now. I'll have a look at the other bug |
|
@marufsharifi I looked into that other bug, and I used the following three CSV files. A-three-rows.csv
It seems there is a bug in the backend where if the Unique ID is less than 5 characters, then it doesn't do any de-duping. Can you confirm if that was the issue with the data you tried?
|
Unique ID was less than 5 characters, then that's okay. thanks. |
|
@tgolen, could you please merge the main. thanks. |
I looked into this a little more, and the reason there is a 5-character limit is that we feel anything less is not large enough to reliably deduplicate. One improvement we could consider is to have it fall back to using transaction information (merchant, date, amount, etc.) to deduplicate transactions. That logic currently exists when there is no Unique ID, but it's bypassed when there is one, and the 5-character limit doesn't fall back to the transaction information. Anyway, I think it's fine for now, but I'm just noting it down for posterity. I'll update the branch now! |
…b-issue-99195-43600a # Conflicts: # src/pages/workspace/companyCards/addNew/CompanyCardsImportedPage.tsx
|
OK, this is updated and retested. There was a pretty big change that got merged into |
|
Hm, the diff shows changes from the |
Resolving the merge with `git add -A` staged the submodule at the commit the working tree happened to have checked out, reverting the pointer bump that came from main. Put main's pointer back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
OK, it's clean now. |
marufsharifi
left a comment
marufsharifi
left a comment
There was a problem hiding this comment.
LGTM, just one minor suggestion.
| // The backend dedupes rows by their `externalID`, so a mapped Unique ID column makes re-uploading | ||
| // the same file idempotent. Without one, every row gets a fresh generated ID and always imports. | ||
| const mappedExternalIDColumnIndex = getColumnIndex(normalizedColumnMappings, CONST.CSV_IMPORT_COLUMNS.EXTERNAL_ID); | ||
| const hasMappedExternalIDColumn = mappedExternalIDColumnIndex >= 0; |
There was a problem hiding this comment.
Boolean naming — [STYLE.md](https://github.com/Expensify/App/blob/main/contributingGuides/STYLE.md#boolean-variables-and-props) requires boolean variables to be prefixed with should or is; has isn't in the sanctioned list ("Use should when we are enabling or disabling some features and is in most other cases").
Renaming hasMappedExternalIDColumn to an is-form and updating its two references:
| const hasMappedExternalIDColumn = mappedExternalIDColumnIndex >= 0; | |
| const isExternalIDColumnMapped = mappedExternalIDColumnIndex >= 0; | |
| if (!isExternalIDColumnMapped) { | |
| normalizedColumnMappings.push(CONST.CSV_IMPORT_COLUMNS.EXTERNAL_ID); | |
| } | |
| const externalIDColumnIndex = isExternalIDColumnMapped ? mappedExternalIDColumnIndex : normalizedColumnMappings.length - 1; |
|
@Beamanator 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] |
Beamanator
left a comment
Beamanator
left a comment
There was a problem hiding this comment.
Overall looking good!
Co-authored-by: Alex Beaman <alexbeaman@expensify.com>
Co-authored-by: Alex Beaman <alexbeaman@expensify.com>
Beamanator
left a comment
Beamanator
left a comment
There was a problem hiding this comment.
Overall looking good!
|
🚧 Beamanator has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.4.64-0 🚀
|
|
Yes — help site changes are required. Draft PR: #99797 This PR adds a user-facing Unique ID column mapping that changes documented behavior in two ways the existing article gets wrong:
Only one article needed changes: What the docs PR changes
Not documented on purpose: the Use advanced fields toggle from this PR's test steps no longer exists — UI label verificationI drove dev NewDot on web to check labels. The flow hit a mandatory account-validation magic code that I can't clear, so the mapping screen itself wasn't reachable. Everything past that point I verified against
Evidence (2)Workspace Company cards page showing the Company cards heading and the Add cards button
Validate your account magic-code screen blocking further progress into the CSV import flow
@tgolen, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
5 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