Fix/70514 - Add payment card RHP opens on the Profile page instead of Subscription#71878
Fix/70514 - Add payment card RHP opens on the Profile page instead of Subscription#71878tgolen merged 15 commits intoExpensify:mainfrom
Conversation
|
@hoangzinh 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] |
|
Hi @tgolen, could you please also assign @WojtekBoman as a reviewer due to #70514 (comment) |
Codecov Report❌ Patch coverage is
... and 10 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| if ( | ||
| matchingFullScreenRoute && | ||
| lastFullScreenRoute && | ||
| (matchingFullScreenRoute.name !== lastFullScreenRoute.name || | ||
| (lastRouteInMatchingFullScreen?.name && lastRouteInLastFullScreenRoute?.name && lastRouteInMatchingFullScreen.name !== lastRouteInLastFullScreenRoute.name)) | ||
| ) { |
There was a problem hiding this comment.
Let's create a separate function to check if full screen routes are equal, it should make this if statement more readable. Do something like !areFullScreenRoutesEqual(matchingFullScreenRoute, lastFullScreenRoute), add this helper function above the component
There was a problem hiding this comment.
Thank you. I have refactored it.
| const isMatchingRoutePreloaded = currentState.preloadedRoutes.some( | ||
| (preloadedRoute) => | ||
| preloadedRoute.name === matchingFullScreenRoute.name && | ||
| (!lastRouteInMatchingFullScreen?.name || | ||
| (preloadedRoute.params && 'screen' in preloadedRoute.params && preloadedRoute.params.screen === lastRouteInMatchingFullScreen?.name)), |
There was a problem hiding this comment.
To me, this one can be simplified as well. Let's create a function isRoutePreloaded and pass currentState and matchingFullScreenRoute as params
There was a problem hiding this comment.
Thank you. I have refactored it.
|
Thanks @dmkt9. I will try review this weekend |
| return currentParams?.reportID === newParams?.reportID; | ||
| } | ||
|
|
||
| function areFullScreenRoutesEqual(matchingFullScreenRoute: NavigationPartialRoute, lastFullScreenRoute: NavigationPartialRoute) { |
There was a problem hiding this comment.
Is it possible to add unit tests for those functions?
There was a problem hiding this comment.
Since the areFullScreenRoutesEqual function is just a small helper used for value comparison, and it's a private function, I don't think it's necessary to create a unit test for it. What do you think?
There was a problem hiding this comment.
We have CodeCov guidelines here. Because we have "decrease coverage" in src/libs/Navigation/helpers/linkTo/index.ts, thus we need to keep or increase it @dmkt9. We either test public method linkTo or export and test areFullScreenRoutesEqual method.
There was a problem hiding this comment.
Thanks. I will update the test soon.
There was a problem hiding this comment.
TY @dmkt9. Leave a comment on PR whenever PR is ready for review again.
|
@hoangzinh This PR is ready. |
|
I found a bug, but it exists in Prod. It shows "It's not here" page when tapping on "Add payment card" in native apps Screen.Recording.2025-10-16.at.17.14.04.android.mov |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-10-16.at.17.14.04.android.movAndroid: mWeb ChromeScreen.Recording.2025-10-16.at.17.11.13.android.chrome.moviOS: HybridAppScreen.Recording.2025-10-16.at.20.24.31.moviOS: mWeb SafariScreen.Recording.2025-10-16.at.17.17.25.ios.safari.movMacOS: Chrome / SafariScreen.Recording.2025-10-16.at.17.03.53.web.movMacOS: DesktopScreen.Recording.2025-10-16.at.17.06.59.desktop.mov |
@hoangzinh Yes. I don’t fully understand the reason, but it seems to be intentional. App/src/pages/settings/Subscription/PaymentCard/index.native.tsx Lines 5 to 15 in e4aabe2 |
|
@blimpich @ikevin127 Do you know why we don't allow adding a payment card for native devices? Is it because of Apple and Google payment rules? I think at least we should disable the button, but I normally prefer to not render things rather than showing them as disabled. |
|
Found this on Slack on a quick look, seems related: Ben Limpich
amy
mitch
|
|
I'm not very familiar with this UI, can you please attach a screenshot of the areas you're asking about? |
|
Thanks for the screenshot. I like this screen being there in general, but I think we should:
cc @Expensify/design |
|
I agree @tgolen!
Can we get some copy help from someone to write this? |
|
cc @jamesdeanexpensify for copy |
|
Did you see this, @tgolen? #70514 (comment) |
|
Oh, no. I didn't. In that case, it looks like this PR and the issue can be closed? It looks like we handle the flow better now for native. |
|
Ah, no @tgolen. The original issue of this PR is: Screen.Recording.2025-11-04.at.22.16.15.movThis PR will ensure that => I think this PR is ready for your review @tgolen |
| return isEqualFullScreenRoute && isEqualLastRouteInFullScreenRoute; | ||
| } | ||
|
|
||
| /** Check whether the route has been preloaded */ |
There was a problem hiding this comment.
You can remove this, it doesn't add value beyond what the function is already named. If you'd like to keep this, I suggest updating the comment to explain why this method exists and when or why it should be used. Also, I'm not sure what "preloaded" means, so that could also be explained in the comment.
There was a problem hiding this comment.
Thanks! I've just removed that line.
|
(Let me know if you need any input on copy, I see I was tagged earlier in this issue!) |
|
@jamesdeanexpensify I think we're OK here and didn't need anything in the end. Thanks! |
|
✋ 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/tgolen in version: 9.2.45-0 🚀
|
|
Hi! This causes #74478 - specifically this commit - mind taking a look quickly, and see if we can fix without a revert? I will also look into it shortly. Thanks! |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.2.45-6 🚀
|

Explanation of Change
Fixed Issues
$ #70514
PROPOSAL: #70514 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Onyx.merge('nvp_private_lastDayFreeTrial', '2025-09-12 01:01:00');(change the date to your current date for the above snippet)
Onyx.merge('nvp_private_amountOwed', 1000);Onyx.merge('nvp_private_billingGracePeriodEnd', 1719792000);They will end the free trial and leave the user in a "expired" state.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Android: Native
android.native.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: Native
ios.native.mp4
iOS: mWeb Safari
ios.mweb.mp4
MacOS: Chrome / Safari
mac.safari.mp4
MacOS: Desktop
mac.desktop.mp4