lindboe commented 11 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 1.4.14-0 Reproducible in staging?: no Reproducible in production?: no If this was caught during regression testing, add the test name, ID and link from TestRail: n/a Email or phone of affected tester (no customers): lizzi@infinitered.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by:@lindboe Slack conversation:

Action Performed:

Open JS console in browser
Create a new manual request (did not reproduce for smartscan)
After creating the request, look at JS console

Expected Result:

No console warnings.

Actual Result:

Console logs a warning:

OnyxUpdates.ts:81 Warning: Failed prop type: Invalid prop `transactions.transactions_6234463839666864617.receipt` of type `array` supplied to `LHNOptionsList`, expected `object`.
    at LHNOptionsList (webpack-internal:///./src/components/LHNOptionsList/LHNOptionsList.js:108:20)
    at withOnyx (webpack-internal:///./node_modules/react-native-onyx/dist/web.development.js:2621:9)
    at withCurrentReportID(Component)
    at div
    at eval (webpack-internal:///./node_modules/react-native-web/dist/exports/View/index.js:52:25)
    at div
    at eval (webpack-internal:///./node_modules/react-native-web/dist/exports/View/index.js:52:25)
    at SidebarLinks (webpack-internal:///./src/pages/home/sidebar/SidebarLinks.js:93:26)
    at div
    at eval (webpack-internal:///./node_modules/react-native-web/dist/exports/View/index.js:52:25)
    at SidebarLinksData (webpack-internal:///./src/pages/home/sidebar/SidebarLinksData.js:85:24)
    at withOnyx (webpack-internal:///./node_modules/react-native-onyx/dist/web.development.js:2621:9)
    at withNetwork(Component)
    at WithNavigationFocus (webpack-internal:///./src/components/withNavigationFocus.tsx:16:91)
    at withCurrentReportID(Component)

I also noticed that the error eventually solves itself. If you log what the LHNOptionsList component is receiving, like this:


function LHNOptionsList({
...
}) {
    console.log("🍊 receipts: ", Object.values(transactions).map(item => item.receipt));
    const styles = useThemeStyles();

You'll see the problem array in the list with a bunch of objects.

If you reload the app, you'll see the error recur, but then the surprising bit: after LHNOptionsList re-renders a few times (visible each time the added console.log statement prints again), eventually the empty array turns into an empty object and the problem goes away.

Workaround:

Can the user still use Expensify without this being fixed? Yes, this is not noticeable to an end user.

Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Video was too large for GitHub, Google drive link: https://drive.google.com/file/d/16yOTJ1n8jQHe0xyjr0Mt9mfBytWGHMw-/view?usp=drive_link (happy to host somewhere else if needed)

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0167764be02a30eae1
Upwork Job ID: 1744424255094456320
Last Price Increase: 2024-01-15

melvin-bot[bot] commented 11 months ago

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 11 months ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

CortneyOfstad commented 11 months ago

I'm not super familiar with JS console errors, so going to get engineering eyes on it so see if it works under a Wave 👍

melvin-bot[bot] commented 11 months ago

Triggered auto assignment to @tylerkaraszewski (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

CortneyOfstad commented 11 months ago

Not overdue

situchan commented 11 months ago

Relevant discussion: https://expensify.slack.com/archives/C01GTK53T8Q/p1699453425518949?thread_ts=1699358032.439529&cid=C01GTK53T8Q Not priority but this is something to be fixed for sure

situchan commented 11 months ago

I can take this as C+

melvin-bot[bot] commented 10 months ago

@tylerkaraszewski, @CortneyOfstad Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 10 months ago

@tylerkaraszewski, @CortneyOfstad Eep! 4 days overdue now. Issues have feelings too...

tylerkaraszewski commented 10 months ago

Giving @situchan the C+ role here.

melvin-bot[bot] commented 10 months ago

@tylerkaraszewski, @CortneyOfstad, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 10 months ago

@tylerkaraszewski, @CortneyOfstad, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 10 months ago

@tylerkaraszewski @CortneyOfstad @situchan this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

CortneyOfstad commented 10 months ago

Not overdue — was OoO for the holidays 👍

melvin-bot[bot] commented 10 months ago

@tylerkaraszewski, @CortneyOfstad, @situchan Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 10 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0167764be02a30eae1

melvin-bot[bot] commented 10 months ago

Current assignee @situchan is eligible for the External assigner, not assigning anyone new.

CortneyOfstad commented 10 months ago

Just realized those labels were not applied, so added them now

hanar3 commented 10 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

After creating a payment request, a prop-types warning mysteriously pops up in the console, the warning disappears later on.

What is the root cause of that problem?

So here's my observations:

The warning shows up after requesting a manual payment (as noted by you guys in the issue)
If we refresh the page once, the warning still pops up
If we refresh the page for a second time, the warning disappears (that's the autofixing behavior described here)

Here's a sane explanation as to why this warning shows up:

When clicking the request money button, we make an RPC call to RequestMoney.
The API call returns an object with a newly created transaction, that transaction has a receipt field, and the receipt field is returned as an empty array (should be an empty object instead)
We store the newly created transaction in the local cache along with the other transactions. Note that because the API returned transaction.receipt as an empty array, that's what we save in the local cache.
We tell React to render transactions

So, after react renders the list of transactions, it realizes that for one transaction, the receipt field is an array, hence the warning.

When we reload the page for the first time after creating the transaction, we use the cached list of transactions (so we can render things faster), while also updating the cache in the background with potentially new information - thing is: the OpenReport RPC (the one that's made in the background and updates the cache) lists all transactions with the correct receipt field (an object, not an array), so after this is done, all the transactions in our cache are correct (meaning all of them have an object, not an array in the receipt field), thus, the warning disappears for subsequent renders.

What changes do you think we should make in order to solve the problem?

The ideal fix is to make a correction to the 'RequestMoney' rpc so that it returns the transaction.receipt as an object rather than an array, like it does for the OpenReport RPC call.
The quick fix is to always force transaction.receipt to be an object by running a data transformation before it gets to LHNOptionsList.

No 1: Won't require a change in the frontend code at all. No 2: Is just meant to be used as a temporary solution as it makes the frontend code a bit harder to understand for no good reason.

Edit: Just using the proper template for a proposal

melvin-bot[bot] commented 10 months ago

📣 @hanar3! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

hanar3 commented 10 months ago

Contributor details Your Expensify account email: vitorhms10@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0110244da8f5486b43

melvin-bot[bot] commented 10 months ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

melvin-bot[bot] commented 10 months ago

@tylerkaraszewski @CortneyOfstad @situchan this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

CortneyOfstad commented 10 months ago

Not overdue

CortneyOfstad commented 10 months ago

@situchan any ideas on the proposals above? Thanks!

melvin-bot[bot] commented 10 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

situchan commented 10 months ago

@hanar3 can you please update proposal based on our codebase? Seems like you proposed after testing only.

hanar3 commented 10 months ago

@hanar3 can you please update proposal based on our codebase? Seems like you proposed after testing only.

@situchan Sure can make an updated proposal, but can you clarify what exactly you want to see updated?

My proposal was not after testing only - I was actively looking at the codebase. I left out most of the technicalities to keep the proposal simple enough, but I'll be happy to point you to the exact lines of code where the data flows through before it gets to the warning, if that's what you expect.

situchan commented 10 months ago

I'll be happy to point you to the exact lines of code where the data flows through before it gets to the warning, if that's what you expect.

yes please

hanar3 commented 10 months ago

@situchan hi again! hopefully this helps clarify things (let me know if it doesn't make sense!)

On src/pages/iou/request/step/IOURequestStepConfirmation.js line 354 the function createTransaction gets called when the Request button is pressed
For a Manual request, the createTransaction function falls in the basecase (line 277), where it calls the requestMoney function.
requestMoney callsIOU.requestMoney, this is what issues the API call (POST to https://dev.new.expensify.com:8082/api?command=RequestMoney) as well as a write to react-native-onyx (the cache). The response of this API call is where the actual problem is! 4.The cache-write happens in the functionSaveResponseToOnyx which is defined in libs/Middleware/SaveResponseToOnyx.ts.

If we put a log to on SaveResponseToOnyx around here:

       // ... SaveResponseToOnyx.ts
        const responseToApply = {
            type: CONST.ONYX_UPDATE_TYPES.HTTPS,
            lastUpdateID: Number(response?.lastUpdateID ?? 0),
            previousUpdateID: Number(response?.previousUpdateID ?? 0),
            request,
            response: response ?? {},
        };

        console.log(responseToApply);

And then make a manual request in the webapp, we can see exactly what's being stored in the cache after an API call - this contains the backend response unchanged, so it's almost the same as just looking at the network tab. (Omitted some things for brevity)

// Result of RequestMoney call below
{
    "transaction": {
      "amount": 100,
      "billable": false,
      "cardID": 0,
      "category": "",
      "comment": {
        "comment": ""
      },
      "created": "2024-01-15",
      "currency": "BRL",
      "externalID": "",
      "filename": "",
      "mcc": "",
      "merchant": "(none)",
      "modifiedAmount": 0,
      "modifiedCreated": "",
      "modifiedCurrency": "",
      "modifiedMCC": "",
      "modifiedMerchant": "",
      "originalAmount": 0,
      "originalCurrency": "",
      "parentTransactionID": "",
      "receipt": [], // -----> The problem is here!
      "reimbursable": true,
      "reportID": "4411715524491711",
      "status": "Posted",
      "tag": "",
      "transactionID": "6427571048478146643"
    },
    "transactionID": "6427571048478146643",
  }
}

This transaction is then merged with the rest of the transactions that are already there. Hopefully you can see the problem now: After the RequestMoney call specifically, the result transaction comes with an empty array in the receipt field (should be an object, that's what we're expecting). That's why I am saying this issue should be ideally be fixed in the backend, not in the frontend. When the "cache merge" happens, is when react is instructed to re-render the list of transactions, which causes prop-types to see a malformed entry in there resulting in the warning.

For reference, the rest of the transactions that are already on the cache come from the OpenReport API call, this returns a list of transactions that looks like this:

// Command=OpenReport
{
    "transactions_1426066331425242734": {
        "amount": 100,
        "billable": false,
        "cardID": 0,
        "category": "",
        "comment": {
            "comment": ""
        },
        "created": "2024-01-08",
        "currency": "BRL",
        "filename": "",
        "merchant": "(none)",
        "modifiedAmount": 0,
        "modifiedCreated": "",
        "modifiedCurrency": "",
        "modifiedMerchant": "",
        "originalAmount": 0,
        "originalCurrency": "",
        "parentTransactionID": "",
        "receipt": {}, // ---> Receipt is an object here, not an array
        "reimbursable": true,
        "reportID": "4411715524491711",
        "status": "Posted",
        "tag": "",
        "transactionID": "1426066331425242734",
        "hasEReceipt": false
    }
}

Since the cache is updated on OpenReport and an API call to that is made when the app first opens, the issue will be auto-fixed after one or two page reloads, because the malformed entry returned from RequestMoney is overridden with a proper transaction entry.

melvin-bot[bot] commented 10 months ago

@tylerkaraszewski @CortneyOfstad @situchan this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Decide whether any proposals currently meet our guidelines and can be approved as-is today
If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 10 months ago

Current assignee @situchan is eligible for the Internal assigner, not assigning anyone new.

CortneyOfstad commented 10 months ago

Not overdue 👍

@situchan any follow-up on the comment here?

CortneyOfstad commented 10 months ago

@situchan any update?

situchan commented 10 months ago

As originally discussed in slack, we should fix this in backend. As all are focused on waves, not prioritized at the moment and bug existed for long time.

situchan commented 9 months ago

Not overdue. Can we move this to Weekly?

CortneyOfstad commented 9 months ago

Not overdue 👍

CortneyOfstad commented 9 months ago

@tylerkaraszewski @situchan — do we think it would be better to adjust this to Monthly instead of Weekly?

CortneyOfstad commented 9 months ago

@lindboe I attempted to recreate this in my test account and could not get anything to populate in the JS console, as shown below:

If the issue pops up again, can you reopen the GH @lindboe? Thank you!

situchan commented 9 months ago

This is dev console error, not happening on production app

CortneyOfstad commented 9 months ago

Oh, thanks for the clarification @situchan! Reopening now 👍

CortneyOfstad commented 8 months ago

Reached out to VIP-Split here

melvin-bot[bot] commented 8 months ago

Current assignee @situchan is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 8 months ago

Current assignee @situchan is eligible for the Internal assigner, not assigning anyone new.

CortneyOfstad commented 8 months ago

Sorry – I was trying to adjust it back to weekly and the tags got all wonky. In regards to this being corrected on the backend, @tylerkaraszewski do you have any updates as to when this may get worked on? Thanks!

CortneyOfstad commented 8 months ago

I am heading OoO (back March 11th) so reassigning BZ to have someone keep an eye on this until I am back. Thanks!

melvin-bot[bot] commented 8 months ago

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

tgolen commented 8 months ago

Daily Update

I've begun debugging and investing this locally.
Slack convo here with my findings so far

Next Step

See if I can figure out where in Auth it's changing from {} to []
Create a fix PR
Merge and deploy

ETA

PR created tomorrow Mar 7
Merged Mar 13
Deployed Mar 14

tgolen commented 8 months ago

Daily Update

I was able to locate where in the code the problem was happening
I've created a PR to fix it

Next Steps

Get PR reviewed/merged/deployed

ETA

Reviewed and merged Monday Mar 11
Deployed to staging Mar 12

CortneyOfstad commented 8 months ago

I'm back from OoO — thanks @kadiealexander for keeping an eye on things!

Expensify / App

[LOW] [Splits] [$500] Console warning: Invalid prop supplied to LHNOptionsList #33268

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Bug0 Triage Checklist (Main S/O)

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

Daily Update

Next Step

ETA

Daily Update

Next Steps

ETA