Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.78k forks source link

[SMARTSCAN] [$500] MEDIUM: Update the workspace chat report preview component when a receipt scan completes #26830

Closed trjExpensify closed 8 months ago

trjExpensify commented 1 year 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!


Action Performed:

Prerequisite: Use the example receipt provided below, because it will likely hit an image parser and complete super fast to test this.

  1. Create a workspace on staging.new.expensify.com
  2. Invite a member to it
  3. As the member, sign-in and navigate to your workspace chat on the workspace
  4. Upload a receipt
  5. Observe the "Receipt scan in progress..." report preview component
  6. Give it 15-30s or so to finish scanning
  7. Navigate away from the workspace chat (i.e tap < to the LHN or click the report preview component)
  8. Navigate back to the workspace chat

Expected Result:

The report preview component should update with the receipt data automatically without needing to navigate away and back to the chat

Actual Result:

The report preview component does not update

Workaround:

Yes, as described above to navigate away and back.

Platforms:

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

Version Number: v1.3.64-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

mWeb safari reproduction: https://github.com/Expensify/App/assets/16232057/019912f4-8c89-415f-92c6-8f1f363a033e

iOS native app reproduction: https://github.com/Expensify/App/assets/16232057/2fa42280-be97-49d9-a62a-5de4699bf94c

Mac Chrome reproduction: https://github.com/Expensify/App/assets/16232057/13172aa7-0abe-435e-aa24-8e25e9225077

Example receipt to use: SaaStr Sept4th Test Receipt

Expensify/Expensify Issue URL: Issue reported by: @trjExpensify Slack conversation: Internal

View all open jobs on GitHub

CC: @Gonals @MariaHCD @JmillsExpensify @dylanexpensify

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01eae90794b2b26fd4
  • Upwork Job ID: 1699178450385817600
  • Last Price Increase: 2023-10-02
Issue OwnerCurrent Issue Owner: @MariaHCD
trjExpensify commented 12 months ago

Ah, I think we can add Reviewing Internal and remove Help Wanted right, Maria?

melvin-bot[bot] commented 12 months ago

@MariaHCD @trjExpensify @0xmiroslav this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 12 months ago

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

trjExpensify commented 12 months ago

Chill, Melv. We have PRs in review.

MariaHCD commented 12 months ago

Continuing to look into this, specifically why the API isn't updating the report total in Onyx: https://github.com/Expensify/Web-Expensify/pull/38930#issuecomment-1742407366

MariaHCD commented 11 months ago

Exploring a different approach for updating the report total, will test tomorrow:

MariaHCD commented 11 months ago

Tests worked as expected. PRs are out for review:

MariaHCD commented 11 months ago

PRs are merged, waiting on deploys so I can retest on staging

mvtglobally commented 11 months ago

Issue not reproducible during KI retests. (First week)

melvin-bot[bot] commented 11 months ago

@MariaHCD, @trjExpensify, @0xmiroslav Whoops! This issue is 2 days overdue. Let's get this updated quick!

MariaHCD commented 11 months ago

I'll retest to see if this is still an issue

kevinksullivan commented 11 months ago

Still seeing this issue and discussing here

https://expensify.slack.com/archives/C05RECHFBEW/p1697645050707899

melvin-bot[bot] commented 11 months ago

@MariaHCD, @trjExpensify, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 11 months ago

@MariaHCD, @trjExpensify, @0xmiroslav Eep! 4 days overdue now. Issues have feelings too...

0xmiros commented 11 months ago

Still in investigation

dylanexpensify commented 11 months ago

@0xmiroslav can you specify exactly what data the frontend needs, but isn't currently receiving from the API? Might help narrow this down

melvin-bot[bot] commented 10 months ago

@MariaHCD, @trjExpensify, @0xmiroslav Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 10 months ago

@MariaHCD, @trjExpensify, @0xmiroslav 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

dylanexpensify commented 10 months ago

bump @0xmiroslav

melvin-bot[bot] commented 10 months ago

@MariaHCD, @trjExpensify, @0xmiroslav Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

0xmiros commented 10 months ago

still investigating. I will provide update by Monday

kevinksullivan commented 10 months ago

Hi @0xmiroslav , still waiting on an update here. Can you please provide one?

0xmiros commented 10 months ago

I think I managed to find all root cause and solution.

BUG1: (frontend) report preview is not re-rendered when transaction pusher event is received so keep scanning

Screenshot 2023-11-15

After fixing BUG1, "keep scanning" issue is gone. But amount shows 0 instead of correct value fetched from receipt. This is because of BUG2 and BUG3 below:

BUG2: (backend) report action pusher event is not received so message[0].text is still old value. i.e. xxx owes $0.00 which expects xxx owes $20.00

https://github.com/Expensify/App/blob/3bbb2142cd3744cc5942ef6494d096c7a2b42ffe/src/components/ReportActionItem/ReportPreview.js#L160-L169

BUG3: (backend) iouReport pusher event is not received so total is still old value as 0.

Fixing BUG2 still doesn't fix the issue completely. It's just fallback when iouReport doesn't exist or iouReport.total = 0 according to this line: https://github.com/Expensify/App/blob/3bbb2142cd3744cc5942ef6494d096c7a2b42ffe/src/components/ReportActionItem/ReportPreview.js#L150-L152

Either fixing BUG2 or BUG3 will fix wrong amount issue but BUG3 is more priority than BUG2 because iouReport has important data of whether to show or hide settlement button:

https://github.com/Expensify/App/blob/3bbb2142cd3744cc5942ef6494d096c7a2b42ffe/src/components/ReportActionItem/ReportPreview.js#L188-L190

Especially this condition: reimbursableSpend !== 0

cc: @MariaHCD

s-alves10 commented 10 months ago

@0xmiroslav

I think this is a backend only issue.

Fixing bug 2 or bug 3 would solve the bug 1 as well We need 2 pusher events to update iou report and report preview action

0xmiros commented 10 months ago

@s-alves10 fixing bug 2 or bug 3 will fix bug 1 because ReportPreview is re-rendered. If transaction pusher event is received separately, this should be issue but I don't think backend will do that. All events will come together when scan complete. So all good.

MariaHCD commented 10 months ago

Thanks, @0xmiroslav! I'm specifically looking into your suggestion of fixing bug 3 which is to ensure the Onyx data for the iouReport receives the updated total.

I think I see the issue. My backend fix here missed accounting for the parser flow. Internals logs. Fix here: https://github.com/Expensify/Web-Expensify/pull/39709, I don't know if there's a way to test it on dev so I can test once it's on staging.

melvin-bot[bot] commented 10 months ago

@MariaHCD, @trjExpensify, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

trjExpensify commented 10 months ago

@MariaHCD were you able to verify this latest backend fix?

MariaHCD commented 10 months ago

Retested again today; looks like we sent the onyx data via pusher but seems like the frontend didn't receive it 😕

Full internal logs:

PudLU8 | 2023-11-27 06:44:50 178 | maria+1@expensifail.com | virt1.lax | src_unknown | [push] Push_Service - Sending event ~~ channel: '[0: 'private-encrypted-user-accountID-8977189' 1: 'private-encrypted-user-accountID-8910926']' eventName: 'onyxApiUpdate' eventData: '[{"onyxMethod":"merge","key":"transactions_463187291062335652","value":{"modifiedAmount":-2000,"modifiedCreated":"2023-09-08","modifiedMerchant":"SaaStr Cantina (E)","modifiedCurrency":"USD","receipt":{"receiptID":675977467,"source":"https:\/\/www.expensify.com\/receipts\/w_268e912cf8b5fbf74b2eee441ac4f2a5ba0c75f0.png","state":"SCANCOMPLETE"}}}]'

But it wasn't reflected in the frontend Onyx DB, the receipt state was still SCANREADY when it was already completed:

Screenshot 2023-11-27 at 10 52 16 AM
PudLU8 | 2023-11-27 06:44:50 128 | maria+1@expensifail.com | virt1.lax | src_unknown | [push] Push_Service - Sending event ~~ channel: '[0: 'private-encrypted-user-accountID-8910926' 1: 'private-encrypted-user-accountID-8977189']' eventName: 'onyxApiUpdate' eventData: '[{"onyxMethod":"merge","key":"report_2891693584086895","value":{"total":-6000}}]'

But on the frontend the report total was not updated:

Screenshot 2023-11-27 at 10 52 29 AM

Seems like something is going wrong with Pusher? Going to have to investigate further.

dylanexpensify commented 10 months ago

Any updates here @MariaHCD? 🙇

MariaHCD commented 10 months ago

Can't really tell why the app doesn't look to be getting those Onyx updates from the API. Asking for help in #eng-chat: https://expensify.slack.com/archives/C03TQ48KC/p1701435063112669

trjExpensify commented 9 months ago

Thanks, @MariaHCD. Looks like Ioni has responded with a coupla' Qs!

MariaHCD commented 9 months ago

From the discussion in Slack, we haven't identified any obvious issues, the pusher events are being sent successfully but not received. Will have to investigate further.

melvin-bot[bot] commented 9 months ago

@MariaHCD, @trjExpensify, @0xmiroslav Eep! 4 days overdue now. Issues have feelings too...

MariaHCD commented 9 months ago

I was retesting this again today and it looks like the Onyx update was received by the App as expected:

For the requestor: The report preview is updated as expected -

https://github.com/Expensify/App/assets/12268372/c2e97bcd-f1a0-4dbc-bb23-3c937dfaa2a1

MariaHCD commented 9 months ago

However, for the payer: I see that the App is receiving the updated transaction data but the report preview isn't being updated, it only updates once the page is reloaded / the report is reopened -

https://github.com/Expensify/App/assets/12268372/a0e4fb0f-c387-4be7-bf3b-e18894fda4e8

@0xmiroslav @s-alves10 any ideas why the updated transaction data isn't being applied for the payer / workspace admin?

MariaHCD commented 9 months ago

After digging into ^ some more, I think the since the report was a whisper, it was not present in the Onyx data for the payer/admin so when the API sends the updated report total, we do not have the complete data to show the report preview.

Screenshot 2023-12-13 at 2 46 02 PM

So, the API should send the whole updated report instead of just the updated report total. I'll work on the PRs for this today.

MariaHCD commented 9 months ago

Pushed both PRs out for review;

melvin-bot[bot] commented 9 months ago

@MariaHCD, @trjExpensify, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

kevinksullivan commented 9 months ago

@MariaHCD is this all set now that those PRs are merged + deployed?

melvin-bot[bot] commented 9 months ago

@MariaHCD, @trjExpensify, @0xmiroslav 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot[bot] commented 9 months ago

@MariaHCD, @trjExpensify, @0xmiroslav Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

melvin-bot[bot] commented 9 months ago

@MariaHCD, @trjExpensify, @0xmiroslav 12 days overdue now... This issue's end is nigh!

MariaHCD commented 9 months ago

Retested this since the relevant PRs were deployed and looks to work better now, the report preview takes a few seconds to update:

https://github.com/Expensify/App/assets/12268372/388c051f-fe76-480a-a006-a900a4a55b91

I think we can close this out but feel free to reopen if this still isn't working well.

MariaHCD commented 8 months ago

@0xmiroslav @s-alves10 Looks like we're still having an issue with the total not being updated in the ReportPreview after the scan completes, the total continues to show $0.00:

https://github.com/Expensify/App/assets/12268372/5a41011c-40fb-4a20-b8f3-4a70071ca709

The pusher update is definitely sent from the backend and received as shown in the console logs. The total was updated for the IOU report in Onyx. But it looks like the App didn't update the report preview. So could it be that the ReportPreview component not updating the total when the pusher event is received?

Screenshot 2024-01-04 at 2 25 58 PM
MariaHCD commented 8 months ago

Although, it looks like we should be calculating the totalDisplaySpend using the total from the IOU report in Onyx:

https://github.com/Expensify/App/blob/d4c3e63956e9c14838592172c7e0e8596b0814cf/src/components/ReportActionItem/ReportPreview.js#L141

So I'm not sure why that doesn't seem to be working 100% correctly. My test on dev worked as expected:

https://github.com/Expensify/App/assets/12268372/6c13e1b0-4771-4ef2-af51-8c661c44b342

s-alves10 commented 8 months ago

@MariaHCD

The current logic for calculating totalDisplaySpend is here https://github.com/Expensify/App/blob/2d455bfb820b13fc83a5dd4057f19516faf2f238/src/libs/ReportUtils.ts#L1714-L1734

In our example, nonReimbursableSpend + totalSpend is equal to 0 and so totalDisplaySpend and reimbursableSpend would be 0

Can you please explain why nonReimbursableSpend is 2000 in this case? What does that mean?

MariaHCD commented 8 months ago

Oh, interesting, so it seems when displayed correctly, the report total is set to the correct amount and nonReimbursableTotal is set at 0:

Screenshot 2024-01-08 at 6 28 56 PM

Not familiar with where/why we set nonReimbursableTotal in the backend, but I'll take a closer look.

MariaHCD commented 8 months ago

Notes for myself:

MariaHCD commented 8 months ago

The backend fixes were deployed to production a while ago so I retested this today and looks to be working better:

https://github.com/Expensify/App/assets/12268372/dae7a3af-48a4-4ce6-99d8-22288d6b7747

https://github.com/Expensify/App/assets/12268372/532fea1a-ac76-425b-a0e1-57a9c65a1eeb