[Wave 8 - ECard Transactions] Update Money Request View Fields to support Ecard transactions #29201

grgia · 2023-10-10T15:18:40Z

Details

Add Pending Field
Add Card Menu Item
Add support for original amount
Move Merchant up based on https://expensify.slack.com/archives/C027726KHT4/p1696943015610349?thread_ts=1696941226.715029&cid=C027726KHT4

Fixed Issues

$ #28835

Tests

Verify that no errors appear in the JS console
We don't currently have card transactions in app, so I tested with mock data.
Create a normal IOU request. Open the transaction thread view for that request. Get the transactionID (I did this by logging it)
Use the following mock data to test cases (in console):

PENDING, with Original Currency

Onyx.merge('cardList',
{
  "4": {
    "bank": "Expensify Card",
    "lastFourPan": "1000",
  }
});

Onyx.merge('transactions_3439799194391522645', {amount: 1000, currency: 'USD', cardID: "4",  modifiedMerchant: null, merchant: "United Airlines", mccGroup: "Goods", created: "2023-07-24 13:46:20", status:"Pending", transactionID: "3439799194391522645", originalAmount: 1000, originalCurrency: "GBP" });

PENDING, with no Original Currency

Onyx.merge('cardList',
{
  4: {
    "bank": "Expensify Card",
    "lastFourPAN": "1000",
  }
});

Onyx.merge('transactions_3439799194391522645', {amount: 1000, currency: 'USD', cardID: 4,  modifiedMerchant: null, merchant: "United Airlines", mccGroup: "Goods", created: "2023-07-24 13:46:20", status:"Pending", transactionID: "3439799194391522645", originalAmount: 0, originalCurrency: '' });

PENDING, with Original Currency

Onyx.merge('cardList',
{
  4: {
    "bank": "Expensify Card",
    "lastFourPAN": "1000",
  }
});

Onyx.merge('transactions_3439799194391522645', {amount: 1000, currency: 'USD', cardID: 4,  modifiedMerchant: null, merchant: "United Airlines", mccGroup: "Goods", created: "2023-07-24 13:46:20", status:"Posted", transactionID: "3439799194391522645", originalAmount: 1000, originalCurrency: "GBP" });

PENDING, with no Original Currency

Onyx.merge('cardList',
{
  4: {
    "bank": "Expensify Card",
    "lastFourPAN": "1000",
  }
});

Onyx.merge('transactions_3439799194391522645', {amount: 1000, currency: 'USD', cardID: 4,  modifiedMerchant: null, merchant: "United Airlines", mccGroup: "Goods", created: "2023-07-24 13:46:20", status:"Posted", transactionID: "3439799194391522645", originalAmount: 0, originalCurrency: '' });

I also went through and verified that the border bottom works for all cases of scanning / posted transactions by manually setting to true/false:
NONE

SCANNING

PENDING

BOTH

Offline tests

QA Steps

Verify that no errors appear in the JS console
Create any kind of manual request, verify that 'Merchant' field is displayed under description.

NO QA for card transactions changes, as we have not started sending card transactions data to App.

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

cloudflare-workers-and-pages · 2023-10-10T15:19:50Z

Deploying with Cloudflare Pages

Latest commit:	`ca92928`
Status:	✅ Deploy successful!
Preview URL:	https://caaf568e.helpdot.pages.dev
Branch Preview URL:	https://georgia-cardtransactions-mon.helpdot.pages.dev

View logs

melvin-bot · 2023-10-10T18:28:24Z

@ArekChr 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]

grgia · 2023-10-11T12:27:35Z

@ArekChr are you able to prioritize this? We have an external commit of Oct 22 for this project :)

melvin-bot · 2023-10-11T16:20:16Z

@ 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]

melvin-bot · 2023-10-11T16:21:07Z

@ 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]

ArekChr · 2023-10-12T09:32:07Z

Hi @grgia, yes, I will do review today

ArekChr

Overall the code looks good to me. I’ve noted a few minor points for consideration in the inline comments.

ArekChr · 2023-10-12T09:46:19Z

src/components/ReportActionItem/MoneyRequestView.js

    } = ReportUtils.getTransactionDetails(transaction);
    const isEmptyMerchant =
        transactionMerchant === '' || transactionMerchant === CONST.TRANSACTION.UNKNOWN_MERCHANT || transactionMerchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT;
    const formattedTransactionAmount = transactionAmount && transactionCurrency && CurrencyUtils.convertToDisplayString(transactionAmount, transactionCurrency);
+    const formattedOriginalAmount = transactionOriginalAmount && transactionOriginalCurrency && CurrencyUtils.convertToDisplayString(transactionOriginalAmount, transactionOriginalCurrency);
+    const isExpensifyCardTransaction = true; // TransactionUtils.isExpensifyCardTransaction(transaction);


The value is hardcoded to true, it appears that the intention was to utilize the TransactionUtils.isExpensifyCardTransaction(transaction)

ArekChr · 2023-10-12T09:49:42Z

src/components/ReportActionItem/MoneyRequestView.js


    const isSettled = ReportUtils.isSettled(moneyRequestReport.reportID);
-    const canEdit = ReportUtils.canEditMoneyRequest(parentReportAction);
+    // NOTE: edit logic for card transactions will be handled in https://github.com/Expensify/App/issues/28836, for now disable editing.


This seems more like TODO

Suggested change

// NOTE: edit logic for card transactions will be handled in https://github.com/Expensify/App/issues/28836, for now disable editing.

// TODO: Temporarily disabling edit functionality for card transactions.

// The logic to handle edit operations for card transactions is pending development and will be addressed in: https://github.com/Expensify/App/issues/28836

ArekChr · 2023-10-12T10:01:20Z

src/libs/CardUtils.ts

@@ -29,7 +29,7 @@ function getMonthFromExpirationDateString(expirationDateString: string) {
 * @param cardID
 * @returns boolean
 */
-function isExpensifyCard(cardID: string) {
+function isExpensifyCard(cardID: number) {


I noticed that the getCardDescription function expects a cardID of type string. Should we also check and adjust the typing on line 44 to number?

ArekChr · 2023-10-12T10:03:15Z

src/libs/TransactionUtils.ts

+}
+
+/**
+ * Return the original amount field from the transaction.


Suggested change

* Return the original amount field from the transaction.

* Return the absolute value of the original amount field from the transaction.

src/libs/TransactionUtils.ts

ArekChr · 2023-10-12T10:16:30Z

Reviewer Checklist

Screenshots/Videos

Onyx.set('cardList', {
            5: {
                bank: 'Expensify Card',
                lastFourPAN: 1000,
            },
        });

Onyx.merge('transactions_5103348747052183509', {
            amount: 1000,
            currency: 'USD',
            cardID: 5,
            modifiedMerchant: null,
            merchant: 'United Airlines',
            mccGroup: 'Goods',
            created: '2023-07-24 13:46:20',
            status: 'Pending',
            transactionID: '5103348747052183509',
            originalAmount: 1000,
            originalCurrency: 'GBP',
        });

Onyx.merge('cardList',
{
  4: {
    "bank": "Expensify Card",
    "lastFourPAN": "1000",
  }
});

Onyx.merge('transactions_5103348747052183509', {amount: 1000, currency: 'USD', cardID: 4,  modifiedMerchant: null, merchant: "United Airlines", mccGroup: "Goods", created: "2023-07-24 13:46:20", status:"Pending", transactionID: "5103348747052183509", originalAmount: 0, originalCurrency: '' });

Onyx.merge('cardList',
{
  4: {
    "bank": "Expensify Card",
    "lastFourPAN": "1000",
  }
});

Onyx.merge('transactions_5103348747052183509', {amount: 1000, currency: 'USD', cardID: 4,  modifiedMerchant: null, merchant: "United Airlines", mccGroup: "Goods", created: "2023-07-24 13:46:20", status:"Posted", transactionID: "5103348747052183509", originalAmount: 1000, originalCurrency: "GBP" });

Onyx.merge('cardList',
{
  "4": {
    "bank": "Expensify Card",
    "lastFourPAN": "1000",
  }
});

Onyx.merge('transactions_5103348747052183509', {amount: 1000, currency: 'USD', cardID: "4",  modifiedMerchant: null, merchant: "United Airlines", mccGroup: "Goods", created: "2023-07-24 13:46:20", status:"Posted", transactionID: "5103348747052183509", originalAmount: 0, originalCurrency: '' });

grgia · 2023-10-12T22:16:19Z

@ArekChr thank you for the review, will update tomorrow morning!

grgia · 2023-10-13T09:10:09Z

@ArekChr, pushed changes!

ArekChr · 2023-10-13T11:15:00Z

@grgia After simulating data, card data is not filled correctly. Could you provide more specific details on where you mocked Onyx?

grgia · 2023-10-13T11:18:38Z

@ArekChr I fixed the data, it should be lastFourPAN capitalized. I also added protection against this in another PR here -

https://github.com/Expensify/App/pull/29474/files#diff-19159f02504c955e4ad7285cc48ac7bfcc0e1cfbec74b371af5cb877311cc9b8

ArekChr · 2023-10-13T11:47:37Z

I changed to the type of number, but it still doesn't help. The card section is not visible; weird 🧐

        Onyx.merge('cardList', {
            4: {
                bank: 'Expensify Card',
                lastFourPan: 1000,
            },
        });

        Onyx.merge('transactions_5103348747052183509', {
            amount: 1000,
            currency: 'USD',
            cardID: 4,
            modifiedMerchant: null,
            merchant: 'United Airlines',
            mccGroup: 'Goods',
            created: '2023-07-24 13:46:20',
            status: 'Pending',
            transactionID: '5103348747052183509',
            originalAmount: 1000,
            originalCurrency: 'GBP',
        });

ArekChr · 2023-10-13T11:49:09Z

Ok, I found a blocker. After logging the transaction, it seems the cardID doesn't change.

grgia · 2023-10-13T11:50:34Z

Ok i found kinda blocker, after logging the transaction it seems the cardID doesn't change at all

@ArekChr why do you have a transaction with that cardID? Curious how you set it before since you have to manual set cardID for testing in general

ArekChr · 2023-10-13T11:50:49Z

@grgia How do you mock data? I think this is essential

grgia · 2023-10-13T11:51:25Z

@ArekChr I use the Onyx merges

ArekChr · 2023-10-13T11:51:59Z

@grgia Yes, I see, but in which place?

grgia · 2023-10-13T11:53:43Z

@ArekChr in the console

grgia · 2023-10-13T11:56:21Z

@ArekChr here's an example of how I use merges to test in action:

Screen.Recording.2023-10-13.at.12.55.15.PM.mov

melvin-bot · 2023-10-13T12:59:31Z

@marcochavezf 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]

ArekChr · 2023-10-13T12:59:38Z

All good, updated checklist 👌

grgia · 2023-10-13T15:56:18Z

@marcochavezf please let me know if you're not able to prioritize this

marcochavezf · 2023-10-13T16:15:56Z

I will review it today

marcochavezf

The code LGTM

marcochavezf · 2023-10-13T17:12:01Z

src/components/MoneyRequestHeaderStatusBar.js

    return (
-        <View style={[styles.dFlex, styles.flexRow, styles.alignItemsCenter, styles.overflowHidden, styles.ph5, styles.pb3, styles.borderBottom, styles.w100]}>
+        <View style={[styles.dFlex, styles.flexRow, styles.alignItemsCenter, styles.flexGrow1, styles.overflowHidden, styles.ph5, styles.pb3, borderBottomStyle]}>


I noticed there are no screenshots for mobile platforms, and I think we'd need to ensure there are no visual regressions introduced in this PR

Gonna merge!

OSBotify · 2023-10-13T18:13:06Z

✋ 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 · 2023-10-13T21:10:16Z

🚀 Deployed to staging by https://github.com/grgia in version: 1.3.84-0 🚀

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

youssef-lr · 2023-10-14T20:39:46Z

src/components/MoneyRequestHeaderStatusBar.js

+    shouldShowBorderBottom: PropTypes.bool.isRequired,
+};
+
+function MoneyRequestHeaderStatusBar({title, description, shouldShowBorderBottom}) {


Heads up this broke the status bar in the split details page and I fixed it here fc1e3ff

OSBotify · 2023-10-16T21:10:41Z

🚀 Deployed to production by https://github.com/francoisl in version: 1.3.84-10 🚀

platform	result
🤖 android 🤖	skipped 🚫
🖥 desktop 🖥	skipped 🚫
🍎 iOS 🍎	skipped 🚫
🕸 web 🕸	skipped 🚫

OSBotify · 2023-10-16T21:47:01Z

🚀 Deployed to production by https://github.com/francoisl in version: 1.3.84-10 🚀

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

OSBotify · 2023-10-16T21:49:57Z

🚀 Deployed to staging by https://github.com/grgia in version: 1.3.85-0 🚀

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

OSBotify · 2023-10-17T23:14:42Z

🚀 Deployed to production by https://github.com/francoisl in version: 1.3.85-4 🚀

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

grgia added 2 commits October 10, 2023 14:20

Correct order of Menu items

64551c9

Update all MoneyRequestView fields

349fe3a

grgia self-assigned this Oct 10, 2023

grgia added 3 commits October 10, 2023 16:59

fix border bottom

ce53bb3

prettier

4708d4d

Merge branch 'main' into georgia-cardTransactions-MoneyRequestView

8eaa030

grgia marked this pull request as ready for review October 10, 2023 18:28

grgia requested a review from a team as a code owner October 10, 2023 18:28

melvin-bot bot requested review from ArekChr and removed request for a team October 10, 2023 18:28

grgia requested a review from a team October 11, 2023 16:20

melvin-bot bot removed the request for review from a team October 11, 2023 16:20

grgia requested a review from a team October 11, 2023 16:21

melvin-bot bot removed the request for review from a team October 11, 2023 16:21

ArekChr reviewed Oct 12, 2023

View reviewed changes

grgia added 2 commits October 13, 2023 10:03

Merge branch 'main' into georgia-cardTransactions-MoneyRequestView

bd3d3da

Remove hardcoded true value, update comments.

4d0a5b1

grgia requested a review from ArekChr October 13, 2023 09:09

Merge branch 'main' into georgia-cardTransactions-MoneyRequestView

594545e

Use format Amount • Original Amount • Status instead

ca92928

ArekChr approved these changes Oct 13, 2023

View reviewed changes

melvin-bot bot requested a review from marcochavezf October 13, 2023 12:59

marcochavezf approved these changes Oct 13, 2023

View reviewed changes

grgia merged commit 1e68e9e into main Oct 13, 2023
18 checks passed

grgia deleted the georgia-cardTransactions-MoneyRequestView branch October 13, 2023 18:06

OSBotify mentioned this pull request Oct 13, 2023

Deploy Checklist: New Expensify 2023-10-13 #29585

Closed

97 tasks

youssef-lr reviewed Oct 14, 2023

View reviewed changes

abdel-h66 mentioned this pull request Oct 14, 2023

[HOLD for payment 2023-10-24] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] Desktop - When uploading a receipt via the request money flow, it shows as Pending #29625

Closed

6 tasks

shubham1206agra mentioned this pull request Oct 15, 2023

[HOLD for payment 2023-10-24] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] Smart scanning header is broken #29626

Closed

6 tasks

OSBotify mentioned this pull request Oct 16, 2023

Deploy Checklist: New Expensify 2023-10-16 #29717

Closed

melvin-bot bot mentioned this pull request Oct 16, 2023

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [ECard Transactions] Update MoneyRequestView UI #28835

Closed

	// NOTE: edit logic for card transactions will be handled in https://github.com/Expensify/App/issues/28836, for now disable editing.
	// TODO: Temporarily disabling edit functionality for card transactions.
	// The logic to handle edit operations for card transactions is pending development and will be addressed in: https://github.com/Expensify/App/issues/28836

	* Return the original amount field from the transaction.
	* Return the absolute value of the original amount field from the transaction.

[Wave 8 - ECard Transactions] Update Money Request View Fields to support Ecard transactions #29201

[Wave 8 - ECard Transactions] Update Money Request View Fields to support Ecard transactions #29201

Conversation

grgia commented Oct 10, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

cloudflare-workers-and-pages bot commented Oct 10, 2023 • edited Loading

Deploying with Cloudflare Pages

melvin-bot bot commented Oct 10, 2023

grgia commented Oct 11, 2023

melvin-bot bot commented Oct 11, 2023

melvin-bot bot commented Oct 11, 2023

ArekChr commented Oct 12, 2023

ArekChr left a comment

Choose a reason for hiding this comment

ArekChr Oct 12, 2023

Choose a reason for hiding this comment

ArekChr Oct 12, 2023

Choose a reason for hiding this comment

ArekChr Oct 12, 2023

Choose a reason for hiding this comment

ArekChr Oct 12, 2023

Choose a reason for hiding this comment

ArekChr commented Oct 12, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

grgia commented Oct 12, 2023

grgia commented Oct 13, 2023

ArekChr commented Oct 13, 2023

grgia commented Oct 13, 2023 • edited Loading

ArekChr commented Oct 13, 2023

ArekChr commented Oct 13, 2023 • edited Loading

grgia commented Oct 13, 2023

ArekChr commented Oct 13, 2023

grgia commented Oct 13, 2023

ArekChr commented Oct 13, 2023

grgia commented Oct 13, 2023

grgia commented Oct 13, 2023

melvin-bot bot commented Oct 13, 2023

ArekChr commented Oct 13, 2023

grgia commented Oct 13, 2023

marcochavezf commented Oct 13, 2023

marcochavezf left a comment

Choose a reason for hiding this comment

marcochavezf Oct 13, 2023

Choose a reason for hiding this comment

grgia Oct 13, 2023

Choose a reason for hiding this comment

OSBotify commented Oct 13, 2023

OSBotify commented Oct 13, 2023

youssef-lr Oct 14, 2023

Choose a reason for hiding this comment

OSBotify commented Oct 16, 2023

OSBotify commented Oct 16, 2023

OSBotify commented Oct 16, 2023

OSBotify commented Oct 17, 2023

grgia commented Oct 10, 2023 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 10, 2023 •

edited

Loading

ArekChr commented Oct 12, 2023 •

edited

Loading

grgia commented Oct 13, 2023 •

edited

Loading

ArekChr commented Oct 13, 2023 •

edited

Loading