[NoQA] Receipt follow up - dedup receipt commands#95811
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@eVoloshchak @mkhutornyi One of you needs to 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
@codex review |
mkhutornyi
left a comment
There was a problem hiding this comment.
Please complete author checklist
ReviewLGTM — this is a safe, behavior-preserving dedup. Verified the consolidation against the actual API param shapes. The set membership is correct. All three consumers (
One intentional behavior change worth calling out in the PR body: before this PR, Minor nits (non-blocking)
Tests are updated consistently and the |
|
No product review needed. |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
Suggested Tests stepsTests — all platforms (web, mWeb, iOS, Android):
Tests — native only (iOS, Android):
|
mountiny
left a comment
There was a problem hiding this comment.
Thanks for following up
|
@cead22 ooops sorry I did not notice you were assigned for a review too 🙈 |
|
🚧 mountiny 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/mountiny in version: 9.4.34-0 🚀
|
|
🤖 No help site changes are required for this PR — no draft PR created. I reviewed the changes in this PR against the help site content under Why no docs update is neededThe PR makes three kinds of changes, none of which alter product behavior or anything a customer would see:
Per the PR description, removing This all concerns internal telemetry/observability and offline receipt-persistence internals — it does not affect any documented feature, setting, tab, or button in the Expensify UI, so there is no corresponding help site article to update. Note: browser-based verification of the live UI was unavailable for this run, so this determination is based on static analysis of the code changes. That's sufficient here since the diff is limited to internal constants, comments, and tests with no behavior or UI change. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.34-14 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.34-14 🚀
Bundle Size Analysis (Sentry): |
Follow up -> #94950 (comment)
Explanation of Change
Deduplicated RECEIPT_BEARING_COMMANDS into a single source of truth, removed outdated entries and simplified the comment.
Why the three split commands were removed
SPLIT_BILL / SPLIT_BILL_AND_OPEN_REPORT – these don't have a top-level receipt (data.receipt). Any receipt is stored inside the splits JSON, so data.receipt is always empty. They were already skipped by this check.
COMPLETE_SPLIT_BILL – this command doesn't include a receipt at all.
These commands were effectively unused in this list. Removing them doesn't change behavior - it just makes the list simpler
Fixed Issues
$ #92139
PROPOSAL:
Tests
Offline tests
QA Steps
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