[Payment due @abzokhattab] Show temporary retry-later message for 531 broken card connection - #98672
Conversation
Map the new brokenCardConnection531 RTER violation type to its own copy telling the user this is a temporary bank issue to retry later, instead of the generic broken-connection message.
This comment has been minimized.
This comment has been minimized.
The main expense violation message comes from the violations.rter translation (via ViolationsUtils.getViolationTranslation), not only from BrokenConnectionDescription. Add a 531 branch there in every language so the temporary retry-later copy shows instead of falling through to the wrong message, and cover it with a getViolationTranslation unit test.
This comment has been minimized.
This comment has been minimized.
Use the same 'Can't auto-match receipt due to a temporary bank issue. Please try again later.' wording for both the rter violation message and the brokenConnection531Error header copy, so the two surfaces show one consistent message.
This comment has been minimized.
This comment has been minimized.
531 was missing from the broken-connection classifiers, so a 531 violation was treated as a generic pending RTER (showing 'Awaiting merge with card transaction.') instead of a broken connection. Add 531 alongside 530 in isBrokenConnectionViolation and hasPendingRTERViolation, and in the MoneyRequestHeader and report status bar broken-connection lookups, so shouldShowBrokenConnectionViolation is true and the 531 copy shows. Covered by a shouldShowBrokenConnectionViolation unit test.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@abzokhattab 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4456a9f543
ℹ️ 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".
|
@MelvinBot @codex review the PR and check for regressions, breaking changes or bugs |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
|
🤖 Reviewed for regressions / breaking changes / bugs. The change is small, additive, and well-tested — the new 🟡
|
|
Should we wait for the backend to be shipped? |
|
@abzokhattab No, I'd actually perfer to roll this one out first. It won't be testable yet until the backend change is done, but I don't want to change the backend until the frontend can support it. |
|
The copy mapping looks correct. My concerns are with the last commit, which reclassifies 531 as a broken connection rather than a pending RTER. 1. Multi-expense reports render no status bar. Excluding 531 from 2. 531 in the personal-card lookup. In 3. Three further behavior changes, none in the test plan. 4. Does Auth #23728 set Tests. No coverage for the Minor.
|
The report status bar's broken-connection branch required a single transaction, so a multi-expense report whose transactions all have a broken-connection violation showed no status and suppressed the next step. Trigger the branch on shouldShowBrokenConnectionViolation and fall back to any report transaction with a broken connection to drive the description. Export isBrokenConnectionViolation for the status bar section.
…sion - Remove 531 from the MoneyRequestHeader and report status bar broken connection lookups; that lookup only drives personal-card suppression, and 531 (like 530) has no fix link, so it should fall through to render the copy instead of being hidden. - Remove the now-dead 531 exclusion in hasPendingRTERViolation; 531 violations never carry pendingPattern, so the exclusion was unreachable. - Use informal 'du' in the German 531 copy to match the surrounding strings. - Add a 530/531 precedence test to BrokenConnectionDescription.
|
Thanks for the thorough review — addressed across d0ec9ba and e7a46c4. #1 (multi-expense status bar): Fixed. The broken-connection branch in #2 (personal-card lookup): Done — dropped 531 from the #3 (product decision): Deliberate call — 531 stays classified as a broken connection, consistent with 530. The loss of the pending-match label and the mark-as-cash offer is the intended behavior for this state, not an accidental side effect of the reclassification. #4 (pendingPattern): Correct — Auth #23728 does not set Tests: Added a 530/531 precedence case to German: Switched the 531 copy to informal "du" to match the surrounding strings. 🤖 Posted by Claude (an AI agent) on behalf of the user. |
Re-review — latest changes look good, and verified live ✅The two new commits since my last pass resolve the mixed-card suppression finding cleanly:
Live test: pass — injecting a
ScreenshotsDebug mode toggled on in Account > Troubleshoot
Expense banner reading the new 531 copy
Expense banner reverted to the original copy after editing rterType back
No further blocking concerns from me — the copy inconsistency and translation-churn items from the earlier pass were already confirmed as intended (Polyglot Parrot output) and are non-blocking. |
|
The latest commit (
No further blocking concerns from me. If you push a new commit, tag me again and I'll re-verify against it. |
|
i think codex reviews are still valid .. what do you think @tgolen |
|
@abzokhattab Thanks! I haven't looked into them yet. I am hoping to have time tomorrow to make sure I understand them and then I'll respond. |
abzokhattab
left a comment
There was a problem hiding this comment.
LGTM 🚀🚀 Thanks for covering the raised issues
|
@youssef-lr 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] |
|
🎯 @abzokhattab, 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. |
|
still trying to get the Jest test to pass 😅 |
|
OK, looks like that's passing now and this is ready for final review. Thanks @abzokhattab! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 youssef-lr 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/youssef-lr in version: 9.4.70-0 🚀
|
|
Yes, help site changes are required. Draft PR: #100339 This PR adds a new user-facing violation message — “Can't auto-match receipt due to a temporary bank issue. Please try again later.” — and the help site only documented broken-connection messaging that tells people to go fix the card. Anyone who searches the new wording currently lands on articles that tell them to reconnect a feed that isn't actually broken, which is the exact confusion the new copy was written to remove. What the docs PR changes (two articles, both under
How the copy was sourced and verifiedEvery quoted message is copied verbatim from The personal card article uses the personal-card wording ( UI labels used in the new copy were verified against the current app UI rather than assumed: Account, Wallet, Cards, and Company cards (lowercase Authored against What I deliberately did not documentThis PR also reworks the report-level status suppression ( @tgolen, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to staging by https://github.com/youssef-lr in version: 9.4.72-0 🚀
|
|
Yes — help site changes are required. Draft PR: #100363 This PR adds a user-facing message that the help site doesn't cover: "Can't auto-match receipt due to a temporary bank issue. Please try again later." Anyone who searches that phrase today lands on the broken-connection articles, which tell them to reconnect a card that isn't actually broken. What I changed
Both say the connection is still valid, no action is needed, and Expensify retries automatically. How I mapped the code to the docs
UI labels used in the new copy — Company cards, Wallet, Fix card — were verified against the live web UI. The @tgolen, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |



Explanation of Change
When a company-card connection returns scrape code
531("expected but unusable response, retry later"), the expense previously showed the generic broken-connection RTER message telling the user to fix their card. Code531is temporary and resolves on its own, so that message is misleading.This PR maps the new backend RTER violation type
brokenCardConnection531to its own copy inBrokenConnectionDescription: "Receipt pending due to a temporary bank issue. Please try again later." The message is added to all languages.This depends on the backend emitting
brokenCardConnection531, which ships in the companion Auth PR: https://github.com/Expensify/Auth/pull/23728Fixed Issues
$ https://github.com/Expensify/Expensify/issues/671466
PROPOSAL:
Tests
The
brokenCardConnection531violation is emitted by the companion Auth PR (not yet deployed), so use Debug mode to inject the violation and verify the copy renders:Account > Troubleshootand enable Debug mode.{ "type": "violation", "name": "rter", "data": { "rterType": "brokenCardConnection531" } }rterTypeback tobrokenCardConnectionand verify the existing broken-bank-connection copy renders instead (no regression).You can also run the automated tests:
npx jest tests/unit/ViolationUtilsTest.ts -t getViolationTranslationnpx jest tests/ui/BrokenConnectionDescriptionTest.tsxOffline tests
The violation copy is rendered from local Onyx data with no network dependency. With the device offline, repeat the Tests steps above and verify the 531 copy still renders correctly.
QA Steps
Same as tests — the Debug-mode steps above can be run on staging. The new copy is otherwise dormant until the linked Auth change is deployed, at which point cards with scrape code 531 surface it in the normal flow.
You can ping @tgolen to QA this.
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