[TS migration] Migrate 'SettingsWalletPhysicalCard' page to TypeScript #33641

pasyukevich · 2023-12-27T11:39:32Z

Details

Fixed Issues

$ #31997
PROPOSAL:

Tests

Go to Settings/Wallet.
add this useEffect on WalletPage.js:

useEffect(() => {
        if (cardList[234523452345]) {
            return;
        }
        // eslint-disable-next-line rulesdir/prefer-actions-set-data
        window.Onyx.merge(`cardList`, {
            234523452345: {
                key: '234523452345',
                cardID: 234523452345,
                state: 2,
                bank: 'Expensify Card',
                availableSpend: 10000,
                domainName: 'expensify.com',
                lastFourPAN: '2345',
                isVirtual: false,
                fraud: null,
            },
        });
    }, [cardList]);

Click on the Expensify Card in the Assigned Cards section list.
On the Expensify Card page you're currently on, press the Get Physical Card button at the bottom of the page.
If you have filled in your details before, you should be shown the confirmation page with the correct data already filled. Otherwise, you'll be shown either the legal name, phone number, or address steps until you have filled in every single piece of data required.
When all data is filled, you can press the Ship Card button on the confirmation step to go back to the Expensify Card page.
At this point, it's not possible to send the request to the API so the Expensify Card page won't show the updated state when you already requested the physical card.

Verify that no errors appear in the JS console

Offline tests

QA Steps

Use an account with Expensify card in 'Not issued' state.
Click on the Expensify Card in the Assigned Cards section list.
On the Expensify Card page you're currently on, press the Get Physical Card button at the bottom of the page.
If you have filled in your details before, you should be shown the confirmation page with the correct data already filled. Otherwise, you'll be shown either the legal name, phone number, or address steps until you have filled in every single piece of data required.
When all data is filled, you can press the Ship Card button on the confirmation step to go back to the Expensify Card page.
At this point, it's not possible to send the request to the API so the Expensify Card page won't show the updated state when you already requested the physical card.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android_native-converted.webm

Android: mWeb Chrome

android_web-converted.webm

iOS: Native

Ios_Native-converted.mp4

iOS: mWeb Safari

Ios_web-converted.mp4

MacOS: Chrome / Safari

mac_web-converted.mov

MacOS: Desktop

mac_desktop-converted.mov

src/libs/GetPhysicalCardUtils.ts

VickyStash · 2023-12-27T14:18:35Z

src/libs/GetPhysicalCardUtils.ts

@@ -116,13 +86,13 @@ function getUpdatedDraftValues(draftValues: DraftValues, privatePersonalDetails:
 * @param draftValues
 * @returns
 */
-function getUpdatedPrivatePersonalDetails(draftValues: DraftValues) {
-    const {addressLine1, addressLine2, city, country, legalFirstName, legalLastName, phoneNumber, state, zipPostCode} = draftValues;
+function getUpdatedPrivatePersonalDetails(draftValues: OnyxEntry<GetPhysicalCardForm | undefined>): PrivatePersonalDetails {


Why do we need undefined inside OnyxEntry value?

Because of onyx values https://github.com/Expensify/App/pull/33641/files#diff-facfc70b32eba19ccaeeadd16b51faf6dbf2ceb919373565e7322c86047886cd

How about removing undefined from the onyx type instead?

Suggested change

function getUpdatedPrivatePersonalDetails(draftValues: OnyxEntry<GetPhysicalCardForm | undefined>): PrivatePersonalDetails {

function getUpdatedPrivatePersonalDetails(draftValues: OnyxEntry<GetPhysicalCardForm>): PrivatePersonalDetails {

Working on that. It is needed to patch a logic also

VickyStash · 2023-12-27T14:20:54Z

src/pages/settings/Wallet/Card/GetPhysicalCardAddress.tsx

-        state: PropTypes.string,
-        zipPostCode: PropTypes.string,
-    }),
+    draftValues: OnyxEntry<GetPhysicalCardForm | undefined>;


Why do we need undefined in this typing?

Because of onyx values https://github.com/Expensify/App/pull/33641/files#diff-facfc70b32eba19ccaeeadd16b51faf6dbf2ceb919373565e7322c86047886cd

@pasyukevich You should rather remove undefined from the onyx value, right?

@blazejkustra I'm not sure about it

Could you clarify your question?

Sorry I'm a bit sick today 😅

Let's remove undefined from ONYXKEYS.ts:

[ONYXKEYS.FORMS.GET_PHYSICAL_CARD_FORM_DRAFT]: OnyxTypes.GetPhysicalCardForm;

src/pages/settings/Wallet/Card/GetPhysicalCardAddress.tsx

src/pages/settings/Wallet/Card/GetPhysicalCardName.tsx

src/pages/settings/Wallet/Card/GetPhysicalCardPhone.tsx

src/libs/GetPhysicalCardUtils.ts

src/pages/settings/Wallet/Card/BaseGetPhysicalCard.tsx

src/pages/settings/Wallet/Card/GetPhysicalCardAddress.tsx

src/pages/settings/Wallet/Card/GetPhysicalCardName.tsx

src/pages/settings/Wallet/Card/GetPhysicalCardPhone.tsx

src/types/onyx/Form.ts

src/libs/CardUtils.ts

melvin-bot · 2024-01-02T17:42:31Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

VickyStash

LGTM

VickyStash · 2024-01-03T07:52:38Z

src/libs/CardUtils.ts

+    const activeCards = Object.values(cardList ?? {}).filter(
+        (card) => !!card?.domainName && (CONST.EXPENSIFY_CARD.ACTIVE_STATES as ReadonlyArray<OnyxTypes.Card['state']>).includes(card.state),


You can use .some instead of .includes to get rid of assertion

kubabutkiewicz · 2024-01-03T10:19:22Z

src/libs/CardUtils.ts

@@ -72,11 +73,13 @@ function getYearFromExpirationDateString(expirationDateString: string) {
 * @param cardList - collection of assigned cards
 * @returns collection of assigned cards grouped by domain
 */
-function getDomainCards(cardList: Record<string, OnyxTypes.Card>) {
+function getDomainCards(cardList: OnyxCollection<OnyxTypes.Card>) {


is this really a OnyxCollection? if not I think we should stick to just Record<string, OnyxTypes.Card>

should it be just OnyxEntry<OnyxTypes.CardList?

Yes, we receive this data from the Onyx

kubabutkiewicz · 2024-01-03T10:25:45Z

src/pages/settings/Wallet/Card/GetPhysicalCardName.tsx

@@ -77,7 +69,7 @@ function GetPhysicalCardName({
                name="legalFirstName"
                label={translate('getPhysicalCard.legalFirstName')}
                aria-label={translate('getPhysicalCard.legalFirstName')}
-                role={CONST.ACCESSIBILITY_ROLE.TEXT}


Q: why those roles changed?

They were replaced as they were deprecated

blazejkustra · 2024-01-08T09:38:45Z

src/pages/settings/Wallet/Card/BaseGetPhysicalCard.tsx

@@ -209,16 +171,14 @@ function BaseGetPhysicalCard({
                onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS_WALLET_DOMAINCARD.getRoute(domain))}
            />
            <Text style={[styles.textHeadline, styles.mh5, styles.mb5]}>{headline}</Text>
-            {renderContent(onSubmit, submitButtonText, styles, children, onValidate)}
+            {renderContent({onSubmit, submitButtonText, children, onValidate})}


Before renderContent function had 5 arguments, now it has one. We need to be careful for any regressions

blazejkustra · 2024-01-08T09:39:47Z

src/pages/settings/Wallet/Card/GetPhysicalCardAddress.tsx

-        state: PropTypes.string,
-        zipPostCode: PropTypes.string,
-    }),
+    draftValues: OnyxEntry<GetPhysicalCardForm | undefined>;


@pasyukevich You should rather remove undefined from the onyx value, right?

blazejkustra · 2024-01-08T09:40:11Z

src/pages/settings/Wallet/Card/GetPhysicalCardConfirm.tsx

@@ -83,7 +56,7 @@ function GetPhysicalCardConfirm({
            submitButtonText={translate('getPhysicalCard.shipCard')}
            title={translate('getPhysicalCard.header')}
        >
-            <Text style={[styles.baseFontStyle, styles.mb5, styles.mh5]}>{translate('getPhysicalCard.estimatedDeliveryMessage')}</Text>


Why these styles were removed?

Only the baseFontStyle was removed, since it is nonexistent in styles

src/pages/settings/Wallet/Card/GetPhysicalCardName.tsx

blazejkustra · 2024-01-12T08:51:44Z

src/ONYXKEYS.ts

-    [ONYXKEYS.FORMS.GET_PHYSICAL_CARD_FORM]: OnyxTypes.Form;
-    [ONYXKEYS.FORMS.GET_PHYSICAL_CARD_FORM_DRAFT]: OnyxTypes.Form | undefined;
+    [ONYXKEYS.FORMS.GET_PHYSICAL_CARD_FORM]: OnyxTypes.GetPhysicalCardForm;
+    [ONYXKEYS.FORMS.GET_PHYSICAL_CARD_FORM_DRAFT]: OnyxTypes.GetPhysicalCardForm | undefined;


Suggested change

[ONYXKEYS.FORMS.GET_PHYSICAL_CARD_FORM_DRAFT]: OnyxTypes.GetPhysicalCardForm | undefined;

[ONYXKEYS.FORMS.GET_PHYSICAL_CARD_FORM_DRAFT]: OnyxTypes.GetPhysicalCardForm;

blazejkustra · 2024-01-12T08:53:05Z

src/libs/GetPhysicalCardUtils.ts

@@ -116,13 +86,13 @@ function getUpdatedDraftValues(draftValues: DraftValues, privatePersonalDetails:
 * @param draftValues
 * @returns
 */
-function getUpdatedPrivatePersonalDetails(draftValues: DraftValues) {
-    const {addressLine1, addressLine2, city, country, legalFirstName, legalLastName, phoneNumber, state, zipPostCode} = draftValues;
+function getUpdatedPrivatePersonalDetails(draftValues: OnyxEntry<GetPhysicalCardForm | undefined>): PrivatePersonalDetails {


How about removing undefined from the onyx type instead?

Suggested change

function getUpdatedPrivatePersonalDetails(draftValues: OnyxEntry<GetPhysicalCardForm | undefined>): PrivatePersonalDetails {

function getUpdatedPrivatePersonalDetails(draftValues: OnyxEntry<GetPhysicalCardForm>): PrivatePersonalDetails {

blazejkustra · 2024-01-12T08:53:43Z

src/pages/settings/Wallet/Card/BaseGetPhysicalCard.tsx

@@ -209,16 +171,14 @@ function BaseGetPhysicalCard({
                onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS_WALLET_DOMAINCARD.getRoute(domain))}
            />
            <Text style={[styles.textHeadline, styles.mh5, styles.mb5]}>{headline}</Text>
-            {renderContent(onSubmit, submitButtonText, styles, children, onValidate)}
+            {renderContent({onSubmit, submitButtonText, children, onValidate})}


Bump @pasyukevich

amyevans · 2024-01-29T18:52:01Z

In addition to the questions above, could you update the QA steps please @pasyukevich? Step 2 won't be able to happen

pasyukevich · 2024-01-31T17:01:42Z

@amyevans As far as I know, currently there are no other possibilities to test it without the hardcode

amyevans · 2024-01-31T22:27:21Z

I think Applause has some accounts with the Expensify card (see the test steps in #35483 for instance).

situchan · 2024-02-05T21:08:53Z

And please resolve conflict @pasyukevich

pasyukevich · 2024-02-06T11:20:26Z

@situchan PR updated

pasyukevich · 2024-02-06T12:14:09Z

@amyevans To test this flow we need an account with a physical card in Not issued state
Applause has a virtual card only.

Where I can check the other options for test accounts?

amyevans · 2024-02-06T20:43:32Z

I'm not sure, I asked in Slack and tagged you in the thread!

pasyukevich · 2024-02-08T11:17:49Z

@amyevans I've updated QA steps for internal testing (according to the discussion in slack)

amyevans · 2024-02-08T14:18:35Z

Alright thanks! Looks like there's still some unresolved questions from @situchan

pasyukevich · 2024-02-08T16:32:42Z

@situchan All comments were addressed

situchan · 2024-02-08T17:01:21Z

Thanks

@amyevans all yours

situchan · 2024-02-08T22:21:09Z

@pasyukevich conflicts

pasyukevich · 2024-02-09T11:24:55Z

@situchan conflicts solved

amyevans

There's a lint error to fix, otherwise looks good though 👍

pasyukevich · 2024-02-09T18:00:21Z

@amyevans Lint fixed

OSBotify · 2024-02-09T18:15:00Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-02-12T14:19:49Z

🚀 Deployed to staging by https://github.com/amyevans in version: 1.4.40-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	failure ❌

kbecciv · 2024-02-12T21:52:21Z

@situchan QA team is unable to see the Expensify card in the 'Not issued' state. in tester@applausecard.expensifail.com account. Can you please verify above PR Internally?

amyevans · 2024-02-12T22:31:11Z

✅ QA'ed it:

qa-33641.mov

OSBotify · 2024-02-13T18:25:10Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.4.40-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	failure ❌

pasyukevich force-pushed the feature/migrate-SettingsWalletPhysicalCard branch 5 times, most recently from 411e55d to c85b80e Compare December 27, 2023 13:48

VickyStash suggested changes Dec 27, 2023

View reviewed changes

pasyukevich force-pushed the feature/migrate-SettingsWalletPhysicalCard branch 6 times, most recently from 6720e7b to 68bad7c Compare December 29, 2023 14:18

VickyStash reviewed Jan 2, 2024

View reviewed changes

pasyukevich force-pushed the feature/migrate-SettingsWalletPhysicalCard branch 3 times, most recently from 052401c to a44bab3 Compare January 2, 2024 17:30

pasyukevich force-pushed the feature/migrate-SettingsWalletPhysicalCard branch from a44bab3 to 3d57b84 Compare January 2, 2024 18:13

VickyStash approved these changes Jan 3, 2024

View reviewed changes

kubabutkiewicz reviewed Jan 3, 2024

View reviewed changes

pasyukevich force-pushed the feature/migrate-SettingsWalletPhysicalCard branch 4 times, most recently from 3a916a3 to 59c7719 Compare January 4, 2024 13:13

blazejkustra suggested changes Jan 8, 2024

View reviewed changes

pasyukevich force-pushed the feature/migrate-SettingsWalletPhysicalCard branch from 59c7719 to 502d7f5 Compare January 10, 2024 13:25

pasyukevich added 2 commits January 10, 2024 17:45

[TS migration] Migrate 'SettingsWalletPhysicalCard' page to TypeScript

079b15d

fix comments

c97ce23

pasyukevich force-pushed the feature/migrate-SettingsWalletPhysicalCard branch from 502d7f5 to c97ce23 Compare January 10, 2024 16:46

blazejkustra suggested changes Jan 12, 2024

View reviewed changes

amyevans requested review from amyevans and removed request for 0xmiros January 29, 2024 15:06

pasyukevich added 2 commits February 6, 2024 12:09

Merge branch 'main' into feature/migrate-SettingsWalletPhysicalCard

35859ad

fix type changes

ab54931

Merge branch 'main' into feature/migrate-SettingsWalletPhysicalCard

eb654da

amyevans reviewed Feb 9, 2024

View reviewed changes

fix lint error

9ebbccd

amyevans approved these changes Feb 9, 2024

View reviewed changes

amyevans merged commit 1c71f13 into Expensify:main Feb 9, 2024
15 checks passed

github-actions bot mentioned this pull request Feb 12, 2024

Deploy Checklist: New Expensify 2024-02-12 #36335

Closed

90 tasks

melvin-bot bot mentioned this pull request Feb 13, 2024

[HOLD for payment 2024-02-20] [TS migration] Migrate 'SettingsWalletPhysicalCard' page to TypeScript #31997

Closed

	function getUpdatedPrivatePersonalDetails(draftValues: OnyxEntry<GetPhysicalCardForm \| undefined>): PrivatePersonalDetails {
	function getUpdatedPrivatePersonalDetails(draftValues: OnyxEntry<GetPhysicalCardForm>): PrivatePersonalDetails {

		const activeCards = Object.values(cardList ?? {}).filter(
		(card) => !!card?.domainName && (CONST.EXPENSIFY_CARD.ACTIVE_STATES as ReadonlyArray<OnyxTypes.Card['state']>).includes(card.state),

	[ONYXKEYS.FORMS.GET_PHYSICAL_CARD_FORM_DRAFT]: OnyxTypes.GetPhysicalCardForm \| undefined;
	[ONYXKEYS.FORMS.GET_PHYSICAL_CARD_FORM_DRAFT]: OnyxTypes.GetPhysicalCardForm;

[TS migration] Migrate 'SettingsWalletPhysicalCard' page to TypeScript #33641

[TS migration] Migrate 'SettingsWalletPhysicalCard' page to TypeScript #33641

Conversation

pasyukevich commented Dec 27, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazejkustra Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melvin-bot bot commented Jan 2, 2024

VickyStash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazejkustra Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amyevans commented Jan 29, 2024

pasyukevich commented Jan 31, 2024

amyevans commented Jan 31, 2024

situchan commented Feb 5, 2024

pasyukevich commented Feb 6, 2024

pasyukevich commented Feb 6, 2024

amyevans commented Feb 6, 2024

pasyukevich commented Feb 8, 2024

amyevans commented Feb 8, 2024

pasyukevich commented Feb 8, 2024

situchan commented Feb 8, 2024

situchan commented Feb 8, 2024

pasyukevich commented Feb 9, 2024

amyevans left a comment

Choose a reason for hiding this comment

pasyukevich commented Feb 9, 2024 • edited Loading

OSBotify commented Feb 9, 2024

OSBotify commented Feb 12, 2024

kbecciv commented Feb 12, 2024 • edited Loading

amyevans commented Feb 12, 2024

OSBotify commented Feb 13, 2024

pasyukevich commented Dec 27, 2023 •

edited

Loading

blazejkustra Jan 12, 2024 •

edited

Loading

blazejkustra Jan 12, 2024 •

edited

Loading

pasyukevich commented Feb 9, 2024 •

edited

Loading

kbecciv commented Feb 12, 2024 •

edited

Loading