-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Add iOS In-App Provisioning #59712
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add iOS In-App Provisioning #59712
Conversation
|
|
|
|
🚧 @mountiny has triggered a test app build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
// stringified {"certificates": string[]} | ||
certificates: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment indicating it looks like this?
"['cert1', 'cert2']"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's more like a result of this: JSON.stringify({"certificates": ['cert1', 'cert2']})
. That's how it works on the OldDot 😅 I think we can change this later on the backend to just ['cert1', 'cert2']
. What do you think about it?
import {checkIfWalletIsAvailable, handleAddCardToWallet, isCardInWallet} from '..'; | ||
import type AddToWalletButtonProps from './types'; | ||
|
||
function RNAddToWalletButton({card, cardHolderName, buttonStyle}: AddToWalletButtonProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably interferes with the import, but perhaps we could alias the import as import { AddToWalletButton as RNWAddToWalletButton }?
src/libs/Wallet/index.ios.ts
Outdated
}) | ||
.catch((e) => { | ||
Log.warn(`isCardInWallet error: ${e}`); | ||
return Promise.resolve(card.state === 6); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this card.state === 6 represent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It represents EXPENSIFY_CARD.STATE.CLOSED
, but I think it's not appropriate here so I will change it to just false
function isCardInWallet(_card: Card) { | ||
// Return true for other platforms, so the AddToWalletButton is always hidden | ||
return Promise.resolve(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? It looks like src/libs/Wallet/RNAddToWalletButton/index.tsx already doesn't return the button so it shouldn't show up regardless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right but we wanted to secure the Wallet functions from using them on unsupported platforms like the web or desktop. It will prevent from bugs in the future and won't make the code platform-specific
🚧 @mountiny has triggered a test app build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
ScreenRecording_04-09-2025.14-11-52_1.mp4ios looks good to me @brunovjk can complete the rest of testing and checklist |
Reviewer Checklist
Screenshots/VideosAndroid: Native59712_android_native.movAndroid: mWeb Chrome59712_android_web.moviOS: NativeiOS: mWeb Safari59712_ios_web.movMacOS: Chrome / Safari59712_web_safari.movMacOS: Desktop59712_web_desktop.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't test the "Add to Wallet Button" itself, because I don't have a physical iOS, but Vit did it here. I tested if the changes did not break anything in the app and LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, all yours @thienlnam
✋ 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/mountiny in version: 9.1.27-0 🚀
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.1.28-15 🚀
|
Explanation of Change
This is a react-native-wallet testing PR for iOS for Expensify App. Some of the In-App Provisioning features are available only on production (on the signed app), so we hid them behind the wallet beta, and we will verify if everything works on production. After that, we will publish the library to npm and change the
tgz
local module for the one from npmImportant
This PR adds an In-App Provisioning feature for iOS only, the Android will be added in the separate PR
Note
The feature is hidden behind the special beta flag, so we can test it properly before launching it for everybody
Fixed Issues
$ #36957
PROPOSAL:
Tests
Note
To be able to test and use In-App Provisioning you need to use physical iOS device on signed app
Settings
>Wallet
Add to Apple Wallet
button is available"<Card title>" will be available in Wallet
)Cancel
and verify if no error appears and the native modal hidesAdd to Apple Wallet
againNest
at the top right cornerAdd to Apple Wallet
button isn't visible on the page when the specified card is already in the wallet. On dev buildCould Not Add Card
alert appears at the end. On prod everything should be fineOffline tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
chrome.mov
iOS: Native
ios_phyiscal.MP4
iOS: mWeb Safari
safari.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov