Fix MoneyRequestTest broken on main after SubscriptionUtils refactor#92291
Conversation
…olicy in test The test relied on shouldRestrictUserBillableActions reading the current user account ID from ONYXKEYS.SESSION via Onyx.connect. PR #91539 removed that implicit default to make the function pure, but the merge into main kept the test's call without the explicit account ID, so it broke on main. Pass TEST_USER_ACCOUNT_ID explicitly, matching the originating PR's test update. Co-authored-by: Tyler Karaszewski <tylerkaraszewski@users.noreply.github.com>
|
@MelvinBot - address the failure in "Spell check" |
cspell flagged the unknown word "assertable" in two comments. Reword to "can be asserted" so the Spell check job passes; reads more naturally and avoids polluting the global cspell dictionary with a niche word. Co-authored-by: Tyler Karaszewski <tylerkaraszewski@users.noreply.github.com>
|
Fixed the failing Spell check check. cspell flagged the unknown word Reworded
|
|
I checked this PR — all checks now pass, so there's no fix to push. Current status: 24 passing, 4 skipped, 0 failing (the only thing still running is the non-blocking The earlier failures were the All code checks — |
|
All checks are now passing — there's nothing to fix. Status: 25 passing, 4 skipped, 0 failing. The only checks that ever failed on this branch were No code change is needed. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
There's nothing to fix here — this PR is already merged and all checks passed (every |
|
🚀 Deployed to staging by https://github.com/chiragsalian in version: 9.3.91-0 🚀
Bundle Size Analysis (Sentry): |
|
@chiragsalian @jjcoffee Any QA steps here? Thank you |
Explanation of Change
test (job 3)started failing onmainwith:This is a semantic merge conflict, not a bug in either branch on its own:
shouldRestrictUserBillableActionspure by removing theOnyx.connect(ONYXKEYS.SESSION)-derived default forcurrentUserAccountID. On that branch, the test intests/actions/IOU/MoneyRequestTest.tswas correctly updated to passTEST_USER_ACCOUNT_IDexplicitly, so the PR's own CI passed.mainconcurrently carried a version of that same test that does not passcurrentUserAccountIDand relied on the now-removed implicit SESSION default. The merge kept the source change andmain's test call (the edits were in different files, so git auto-merged cleanly without flagging a conflict).Result on
main: the implicit default is gone, the test passesundefinedforcurrentUserAccountID, the policy-owner check fails, restriction returnsfalse, andshouldUseDefaultExpensePolicyreturnstrueinstead offalse.This PR restores the explicit
TEST_USER_ACCOUNT_IDargument that PR #91539 already had on its branch, which is the contract the now-pure function requires.Fixed Issues
$ #92283
PROPOSAL: #92283 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
N/A — test-only change.
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
AI Tests (run by MelvinBot)
npm test -- tests/actions/IOU/MoneyRequestTest.ts(shouldUseDefaultExpensePolicy block) — ✅ pass (previously-failing test now green)npm run lint-changed— ✅ passnpm run typecheck-tsgo— ✅ passnpm run prettieron the changed file — ✅ no changesnpm testsuite, storybook smoke test.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
N/A — test-only change.