Skip to content
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

[HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$500] Distance - Distance request thumbnail is different in reports when created offline #37638

Closed
6 tasks done
lanitochka17 opened this issue Mar 1, 2024 · 55 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Mar 1, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.46-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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go offline
  3. Go to workspace chat
  4. Create a distance request
  5. Click on the expense preview
  6. Click on the expense preview again

Expected Result:

The receipt preview for the distance request should be consistent in the main chat, expense report and transaction thread

Actual Result:

The receipt preview for the distance request is different in the main chat, expense report and transaction thread

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6398750_1709323958826.bandicam_2024-03-02_01-11-44-819.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01230fd2c4938cb72a
  • Upwork Job ID: 1765104893993635840
  • Last Price Increase: 2024-03-19
  • Automatic offers:
    • mollfpr | Reviewer | 0
    • FitseTLT | Contributor | 0
Issue OwnerCurrent Issue Owner: @garrettmknight
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 1, 2024
Copy link

melvin-bot bot commented Mar 1, 2024

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

@lanitochka17
Copy link
Author

@garrettmknight FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@lanitochka17
Copy link
Author

We think that this bug might be related to #wave5
CC @dylanexpensify

@rayane-djouah
Copy link
Contributor

rayane-djouah commented Mar 1, 2024

Proposal

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

Distance request thumbnail is different in reports when created offline.

What is the root cause of that problem?

when creating the distance request we set an optimisticReceipt with source as ReceiptGeneric image here:

App/src/libs/actions/IOU.ts

Lines 992 to 995 in fc4297d

const optimisticReceipt: Receipt = {
source: ReceiptGeneric as ReceiptSource,
state: CONST.IOU.RECEIPT_STATE.OPEN,
};

and we are using ReceiptGeneric also when displaying the receipt here:
return {thumbnail: null, image: ReceiptGeneric, isLocalFile: true};

that's because we have only svg icon of the route pending image that we are using for example here. we couldn't use Expensicons.EmptyStateRoutePending here and here. because of the type mismatch.

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

we should add png version for route pending image
and use it as source when creating the optimisticReceipt here and when displaying the image here

@gijoe0295
Copy link
Contributor

gijoe0295 commented Mar 1, 2024

Proposal

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

Distance - Distance request thumbnail is different in reports when created offline

What is the root cause of that problem?

We display ReceiptGeneric on preview for distance request that is pending route from server:

if (TransactionUtils.isFetchingWaypointsFromServer(transaction)) {
return {thumbnail: null, image: ReceiptGeneric, isLocalFile: true};
}

But we use EmptyStateRoutePending icon for others.

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

We should add png image for EmptyStateRoutePending because it's currently just SVG.

Then change ReceiptGeneric above to EmptyStateRoutePending for consistency.

What alternative solutions did you explore? (Optional)

NA

@melvin-bot melvin-bot bot added the Overdue label Mar 4, 2024
Copy link

melvin-bot bot commented Mar 5, 2024

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

1 similar comment
Copy link

melvin-bot bot commented Mar 5, 2024

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

Copy link

melvin-bot bot commented Mar 5, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01230fd2c4938cb72a

@melvin-bot melvin-bot bot changed the title Distance - Distance request thumbnail is different in reports when created offline [$500] Distance - Distance request thumbnail is different in reports when created offline Mar 5, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 5, 2024
Copy link

melvin-bot bot commented Mar 5, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External)

@garrettmknight
Copy link
Contributor

Confirmed and added to a wave.

@shahinyan11
Copy link

shahinyan11 commented Mar 5, 2024

Proposal

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

Distance - Distance request thumbnail is different in reports when created offline

What is the root cause of that problem?

We display another component here

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

Add && !isOffline in this condition

const {isOffline} = useNetwork();
...

const showMapAsImage = isDistanceRequest && hasPendingWaypoints && !isOffline;

What alternative solutions did you explore? (Optional)

@FitseTLT
Copy link
Contributor

FitseTLT commented Mar 5, 2024

Proposal

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

Distance request thumbnail is different in reports when created offline

What is the root cause of that problem?

We are using ConfirmRoute in money request preview and money request view

<ConfirmedRoute transaction={transaction} />
</View>
)}

<ConfirmedRoute transaction={transaction} />
) : (

when the route is pending for a distance request and we do this to render the map same as in confirmation page instead of waiting until the server returns the map image and the default behaviour of this image when offline is to return PendingMapView which displays EmptyStateRoutePending Icon

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

To be consistent with ReportPreview as in here

{hasReceipts && (
<ReportActionItemImages
images={lastThreeReceipts}
total={transactionsWithReceipts.length}
size={CONST.RECEIPT.MAX_REPORT_PREVIEW_RECEIPTS}
/>
)}

we should display the ReportActionItemImage with the placeholder Receipt thumbnail instead of ConfirmRoute when we are offline as in that case the ConfirmRoute will display EmptyStateRoutePending icon that doesn't align with ReportPreview causing this issue
So we need to change these conditions
const showMapAsImage = isDistanceRequest && hasPendingWaypoints;

const showMapAsImage = isDistanceRequest && hasPendingWaypoints;

   const showMapAsImage = isDistanceRequest && hasPendingWaypoints && !isOffline;

What alternative solutions did you explore? (Optional)

But in the other way round we can also use ConfirmRoute for Report Preview that is inside ReportActionItemImages instead of displaying always REportActionItemImage we can display ConfirmRoute when the transaction is distance request and has pending waypoints

<ReportActionItemImage
thumbnail={thumbnail}
image={image}
isLocalFile={isLocalFile}
transaction={transaction}
isSingleImage={numberOfShownImages === 1}
/>

In this case we will have the advantage of ConfirmRoute (i.e to avoid latency in the display of map view until the BE returns it) in the Report preview too

@melvin-bot melvin-bot bot added the Overdue label Mar 7, 2024
@garrettmknight
Copy link
Contributor

@mollfpr can you check these out?

@melvin-bot melvin-bot bot removed the Overdue label Mar 7, 2024
@garrettmknight garrettmknight removed the Monthly KSv2 label Apr 23, 2024
@garrettmknight
Copy link
Contributor

Still working through the PR

@neil-marcellini
Copy link
Contributor

Chirag said I can take this over, so I will and I think we can get it merged in the next few days.

@neil-marcellini
Copy link
Contributor

We encountered various bugs in the PR and have had to determine if each is new or existing on main already. I'll set a reminder to review the PR again tomorrow, but we'll probably have to keep waiting for a bit due to the merge freeze.

@garrettmknight
Copy link
Contributor

garrettmknight commented May 30, 2024

Still working through the PR - I think it's ready for another review @mollfpr

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jun 11, 2024
@melvin-bot melvin-bot bot changed the title [$500] Distance - Distance request thumbnail is different in reports when created offline [HOLD for payment 2024-06-18] [$500] Distance - Distance request thumbnail is different in reports when created offline Jun 11, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jun 11, 2024
Copy link

melvin-bot bot commented Jun 11, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Jun 11, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.81-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-06-18. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Jun 11, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
  • [@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@mollfpr] Determine if we should create a regression test for this bug.
  • [@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@garrettmknight] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jun 13, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-06-18] [$500] Distance - Distance request thumbnail is different in reports when created offline [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$500] Distance - Distance request thumbnail is different in reports when created offline Jun 13, 2024
Copy link

melvin-bot bot commented Jun 13, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.82-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-06-20. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Jun 13, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
  • [@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@mollfpr] Determine if we should create a regression test for this bug.
  • [@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@garrettmknight] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jun 18, 2024
@garrettmknight
Copy link
Contributor

garrettmknight commented Jun 20, 2024

Payment Summary:

@mollfpr can you finish the checklist when you get a chance?

@mollfpr
Copy link
Contributor

mollfpr commented Jun 22, 2024

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

Couldn't find the offending PR. We re-work the component to fix the issue and also replace the map pending thumbnail.

[@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

No need to create a discussion on the channel, because it seems only a time bug.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

  1. Go to staging.new.expensify.com
  2. Go offline
  3. Go to workspace chat
  4. Create a distance request
  5. Verify on the report preview check that the pending map is displayed corresponding to the distance request created in (4) (instead of the receipt thumbnail)
  6. Get back online and verify that the pending map is displayed (instead of the receipt thumbnail) until the map image from the server is returned
  7. And once the map image is ready it should display the map image.
  8. Create a distance request again
  9. Verify on the report preview check a map is displayed corresponding to the distance request u created (instead of a receipt thumbnail)
  10. 👍 or 👎

@JmillsExpensify
Copy link

$500 approved for @mollfpr

@melvin-bot melvin-bot bot added the Overdue label Jun 24, 2024
@garrettmknight
Copy link
Contributor

All set here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Development

No branches or pull requests