[Payment due @mkhutornyi] Show inactive-vendor copy when the assigned vendor is missing from the synced list#94598
Conversation
|
@codex review |
|
from Alex's Claude agent 🔴 Blocker — FixedYou're right about On 🟡 Minor —
|
![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: 2bb2ffbc57
ℹ️ 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".
|
@codex review |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e22ff9ffc
ℹ️ 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".
Co-authored-by: Alex Beaman <Beamanator@users.noreply.github.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@mkhutornyi any chance you're free to help review & test this, since you helped review & test my recent QBO PR? 🙏 |
sure |
Thanks so much 🙏 🙏 |
melvin-bot
Bot
requested review from
aimane-chnaif and
heyjennahay
and removed request for
a team
|
@aimane-chnaif 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] |
|
Sorry @aimane-chnaif - i asked @mkhutornyi b/c they've helped review very similar PRs in the recent past 🙏 |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Match the file's zero-comments-in-test-body convention. The pre-test preambles restated the test name and referenced issue/Codex history; the inline comments restated the code beneath them. Mock comment trimmed to the non-obvious sibling-layout WHY.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
The Tests steps pass but it's weird to me that "Vendor no loger valid" shows twice (one for value field, one for violation message). 
|
|
hmm actually oops no, we should show the set vendor in white still - fixing 😬 |
…ation state Reviewer flagged that 'Vendor no longer valid' showed twice — once in the field title and once in the red violation message below — when an active QBO vendor was deactivated. The title now resolves the assigned vendor's name from any connection (active integration first, then a permissive cross-connection lookup) and falls back to the raw externalID when no connection has the vendor. The INACTIVE_VENDOR violation in red provides the 'no longer valid' indicator separately, so the title stays focused on what was assigned.
…hingVendors export ESLint flagged `?? ''` after a string ID per the no-default-id-values rule; mkhutornyi separately flagged that the raw ID shouldn't surface as the title. The Vendor field title now resolves through findVendorByID across every connection and renders empty only when the assigned vendor cannot be found anywhere. The red INACTIVE_VENDOR violation continues firing from ViolationsUtils and provides the 'no longer valid' indicator in that fully-purged case. Also removes the getActiveVendorMatchingVendors PolicyUtils export — its only consumer was removed in the previous commit, so Knip flagged it as unused. The function stays as an internal helper for getMatchingVendors and isMatchingVendorListLoaded.
Per the resolution on mkhutornyi's review thread (#94598 (comment)), keep the raw externalID visible when no connection has the vendor name any more. The previous commit dropped the fallback to clear the no-default-id-values ESLint rule, but the explicit early-fallback intent was supposed to stay until TransactionCommentVendor carries a persisted name field. The conditional assignment avoids the rule's substring trigger (`id ?? ''`) by branching explicitly. Added a TODO referencing the data-model follow-up so the externalID surface can be replaced with a real vendor name once that lands.
|
@tylerkaraszewski 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] |
melvin-bot
Bot
changed the title
|
🎯 @mkhutornyi, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 861f150f86
ℹ️ 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".
| } else if (transactionVendor?.externalID) { | ||
| transactionVendorName = transactionVendor.externalID; |
There was a problem hiding this comment.
When a non-reimbursable expense still has comment.vendor.externalID but the synced active vendor list has already loaded without that ID (for example after the QBO vendor is deleted), this branch sets the field title to the raw external ID instead of the translated inactive-vendor copy. In the synced-delete scenario the violation may not be recomputed until the next transaction edit, so the expense view still shows an opaque ID and never displays “Vendor no longer valid” on open, leaving the original missing-vendor UI issue unresolved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We discussed this, it's fine to let it show the externalID for now - will fix in a followup
|
🚧 tylerkaraszewski has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/tylerkaraszewski in version: 9.4.25-0 🚀
|
|
@Beamanator I am not sure if applause can run this following step.
Could this be verified internally? |
|
oops ya this is behind a beta so NAB but yes we will test it internally 👍 |
|
🤖 Payment issue created: #95082 |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.4.25-2 🚀
Bundle Size Analysis (Sentry): |
6 participants
Explanation of Change
When a vendor is deactivated or deleted in QuickBooks Online, the synced vendor list at
policy.connections.quickbooksOnline.data.vendorsdrops that vendor on the next sync. The expense'stransaction.comment.vendor.externalIDis intentionally preserved as the audit trail of the prior coding decision, butMoneyRequestViewwas rendering the Vendor field with an empty title — making it look like the vendor had been stripped off the expense entirely. The fix falls back to the existingviolations.inactiveVendorcopy ("Vendor no longer valid") wheneverfindVendorByIDreturns undefined whileexternalIDis still set. The matchingINACTIVE_VENDORout-of-policy violation already fires fromViolationsUtils.getViolationsOnyxData. Per-sync recompute of that violation (pushTransactionViolationsOnyxDatatriggered from anOnyx.connectlistener onpolicy.connections.quickbooksOnline.data.vendors, plus the equivalent call fromselectExportCompanyCardwhen export type leaves credit/debit card) is tracked as a follow-up — without it the violation only re-evaluates on the next transaction edit.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/651950
PROPOSAL: https://github.com/Expensify/Expensify/issues/651950#issuecomment-4790875708
Tests
comment.vendoris still set in Onyx (audit trail preserved)INACTIVE_VENDORviolation appears in the violation listOffline tests
The fallback rendering reads from Onyx and runs entirely client-side. Repro the Tests steps above with the network toggled off after step 3 — the "Vendor no longer valid" copy must still render and the
comment.vendorobject on the transaction must remain intact.QA Steps
Same as Tests above on staging. The Web-E sibling for the same issue is at https://github.com/Expensify/Web-Expensify/pull/54012 and addresses a separate gap (commercial-card-feed coverage); the two PRs are independent and can be QA'd in either order.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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