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-09-06] [$250] mWeb - Submit expense - Site crashes sending a distance expense after two scan expenses #46296

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

Comments

@lanitochka17
Copy link

lanitochka17 commented Jul 26, 2024

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: 9.0.13-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: https://expensify.testrail.io/index.php?/tests/view/4765667
Email or phone of affected tester (no customers): nahummelaku9+d27@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

  1. Login to staging.new.expensify.com
  2. Navigate to FAB > Submit expense > Scan
  3. Upload a receipt and click on next
  4. Choose a workspace and submit it
  5. Navigate to FAB>QAB(Scan IOU)
  6. Upload a receipt and submit it to the same workspace
  7. Navigate to FAB>Submit expense > Distance
  8. Enter start and finish locations and click next
  9. Select the same workspace you submitted the scan expenses to and click submit

Expected Result:

Distance expense is created and navigated to the conversation

Actual Result:

The site crashes with a message "Uh-oh, something went wrong!"

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

Bug6552463_1721899827253.video_2024-07-25_12-17-52.mp4

logs (2).txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01890de5ca6ea4b22c
  • Upwork Job ID: 1818469788247083194
  • Last Price Increase: 2024-08-07
  • Automatic offers:
    • ikevin127 | Reviewer | 103438207
    • ShridharGoel | Contributor | 103438209
Issue OwnerCurrent Issue Owner: @ikevin127
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 26, 2024
Copy link

melvin-bot bot commented Jul 26, 2024

Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@ShridharGoel
Copy link
Contributor

ShridharGoel commented Jul 26, 2024

Proposal

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

Site crashes when distance map calls fitBounds with high padding on low screen size.

What is the root cause of that problem?

This happens when fitBounds is called with high padding, but the screen size is small.

map.fitBounds([northEast, southWest], {padding: mapPadding});

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

We can make the mapPadding dependent on the screen size.

In usages of DistanceMapView, update mapPadding to below:

mapPadding={getIsNarrowLayout ? 0 : CONST.MAP_PADDING}

This is used in ConfirmedRoute and DistanceRequestFooter.

Else, we can put the getIsNarrowLayout check while calling fitBounds.

@bernhardoj
Copy link
Contributor

Proposal

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

The app crash when adding a distance request after 2 scan request.

What is the root cause of that problem?

When we have 2 scan expense + 1 distance, the size of the preview becomes small.
image

Then, we set a padding of 50 to the map which will be used when trying to center the map.

mapPadding={CONST.MAP_PADDING}

map.fitBounds([northEast, southWest], {padding: mapPadding});

However, the size of the map is smaller than the padding combined (left+right / top+bottom).

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

Change the padding to the smallest size (or 0 or decrease the value) if it's bigger than the map size.

let newMapPadding = mapPadding;
if (mapPadding && mapPadding > 0 && (mapPadding * 2 > mapRef.getCanvas().clientHeight || mapPadding * 2 > mapRef.getCanvas().clientWidth)) {
    newMapPadding = Math.min(mapRef.getCanvas().clientHeight, mapRef.getCanvas().clientWidth) / 2; // or 0
}
map.fitBounds([northEast, southWest], {padding: newMapPadding});

@melvin-bot melvin-bot bot added the Overdue label Jul 29, 2024
Copy link

melvin-bot bot commented Jul 29, 2024

@greg-schroeder Whoops! This issue is 2 days overdue. Let's get this updated quick!

@marcaaron marcaaron self-assigned this Jul 31, 2024
@melvin-bot melvin-bot bot removed the Overdue label Jul 31, 2024
@marcaaron marcaaron added External Added to denote the issue can be worked on by a contributor Overdue labels Jul 31, 2024
@melvin-bot melvin-bot bot changed the title mWeb - Submit expense - Site crashes sending a distance expense after two scan expenses [$250] mWeb - Submit expense - Site crashes sending a distance expense after two scan expenses Jul 31, 2024
Copy link

melvin-bot bot commented Jul 31, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01890de5ca6ea4b22c

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 31, 2024
Copy link

melvin-bot bot commented Jul 31, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Jul 31, 2024
@ikevin127
Copy link
Contributor

♻️ Reviewing proposals today.

@ikevin127
Copy link
Contributor

ikevin127 commented Aug 1, 2024

@ShridharGoel's proposal looks good to me. They were first to identify the root cause and the proposed solution fixes the issue. However, there's some important context to understand here before moving forward with a fix - and I think @Expensify/design might want to weigh in here regarding the best choice 👇

Context

Notes:

  • I reproduced / tested this on MacOS Chrome using Toggle devices toolbar which allowed me to test on different phones which have different screen sizes (width / height).
  • On narrow layout screens I noticed that the site crash happens around the padding value 40-42 and over, depending on device screen size.
  • I tested from Samsung Galaxy S8+ (360x740) to iPhone 4 (320x480) to iPhone 5 (320 x 568) and the best looking value that doesn't crash the app even on iPhone 4 is 20 Padding.

Here's how the map preview looks on narrow layouts with different values vs on large layouts:

0 Padding 20 Padding 40 Padding (crashes on smaller devices) 50 Padding (Large Layout)
p-0 p-20 p-40 large-layout

As one can see, the lower the padding, the more zoomed-in the map preview is and vice-versa.
This applies on the individual distance request preview as well:

20 Padding (Narrow Layout)
Screenshot 2024-07-31 at 18 23 35

Given the above ^ context, I think that going with @ShridharGoel's proposal is the right path, with some slight changes:

  • like we have CONST.MAP_PADDING for large layout devices which is currently used for narrow layouts as well (causing the crash), instead of having 0 Padding for narrow layouts, have a new CONST variable with 20 Padding value, something like CONST.MAP_PADDING_NARROW (or similar)
  • use CONST.MAP_PADDING_NARROW if getIsNarrowLayout() is true

Important

We should wait for design's team input here before moving forward.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Aug 1, 2024

Current assignee @marcaaron is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@ikevin127
Copy link
Contributor

⚠️ @bernhardoj I tested your proposed solution on the following narrow layout device: Samsung Galaxy S8+ (360x740) and the calculated newMapPadding was 42 which was still crashing the app. Check out my previous comment for more context on how this padding affects different screen sizes which still crash the app if the padding is over a certain value (which differs based on device screen size).

@ShridharGoel
Copy link
Contributor

@shawnborton Can you have a look? #46296 (comment)

@shawnborton
Copy link
Contributor

Oh wow, great investigation! So just to be clear, what are you recommending we use for the paddings for narrow screens vs wide screens? Personally I don't think there needs to be a huge difference, given that the size of the map preview is pretty much the same on mobile vs desktop.

Also, I think this "more zoomed in" screenshot looks good IMO, but would love to see it with a route that is horizontal too.
CleanShot 2024-08-01 at 06 04 53@2x

cc @dubielzyk-expensify (skipping Danny ping while he's 🌴) for vis

@ShridharGoel
Copy link
Contributor

@shawnborton For wide screens, we can keep the existing padding of 50.

For narrow screens, either we can just go to 0, or we can set it to 20 based on @ikevin127's testing.

@shawnborton
Copy link
Contributor

Can you show me the side-by-side difference though? The receipt thumbnail is virtually the same size on both wide and small screens, so I'm not sure if we actually want a difference here.

@ikevin127
Copy link
Contributor

I stand by my previous #46296 (comment) and considering the context from that comment:

Note

Based on the results below I think we should keep 50 Padding on wide screens layout and adjust to 30 Padding only for narrow screens layout - this would ensure things look good on narrow layout devices and also keep things within scope as the main thing here is to ensure the app doesn't crash.

Here's the side by side in different scenarios:

Note:

  1. I left out the ones where there's no difference between 20 vs 50 Padding (longer routes).
  2. Keep in mind the context shown regarding the shorter vertical route from my comment above.

1. 20 vs 50 Padding - Wide screen layout on longer horizontal / vertical routes

Caution

This is why I think we shouldn't touch the wide screen layout value of 50 Padding.

Flow: Submit expense (Waypoints) Screen

20 Padding - Horizontal Route 50 Padding - Horizontal Route
start-wp-20-hz start-wp-50-hz

Flow: Confirm Distance expense Screen

20 Padding - Horizontal Route 50 Padding - Horizontal Route 20 Padding - Vertical Route 50 Padding - Vertical Route
confirm-20-hz confirm-50-hz confirm-20-vt confirm-50-vt

2. 20 vs 50 Padding - Narrow screeen layout on longer horizontal / vertical routes

Caution

Here we have to decrease the padding because otherwise the app will crash.

Flow: Submit expense (Waypoints) Screen

20 Padding - Horizontal Route 50 Padding - Horizontal Route 20 Padding - Vertical Route 50 Padding - Vertical Route
start-wp-20-hz start-wp-50-hz start-wp-20-vt start-wp-50-vt

Flow: Confirm Distance expense Screen

20 Padding - Horizontal Route 50 Padding - Horizontal Route 20 Padding - Vertical Route 50 Padding - Vertical Route
confirm-20-hz confirm-50-hz confirm-20-vt confirm-50-vt

@ShridharGoel Given the results on narrow layout, I think the best option would be to go with 30 Padding which is closer to 50 but not close enough to crash the app. I tested with 30 Padding on the smaller device: iPhone 4 (320x480) and it doesn't crash. @shawnborton Wdyt ?

Copy link

melvin-bot bot commented Aug 6, 2024

@greg-schroeder, @marcaaron, @ikevin127 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Aug 6, 2024
@ikevin127
Copy link
Contributor

Awaiting input from the design team on how we want the UI to look, then will be able to move on with opening a PR.

@melvin-bot melvin-bot bot removed the Overdue label Aug 10, 2024
@izarutskaya
Copy link

This issue reproducible also after third distance request. Reproduce in production
Let me know please if need to create a new GH. Thanks

Screenrecorder-2024-08-13-12-28-01-781_compress_1.mp4

@melvin-bot melvin-bot bot added the Overdue label Aug 13, 2024
@ShridharGoel
Copy link
Contributor

ShridharGoel commented Aug 13, 2024 via email

@greg-schroeder
Copy link
Contributor

Thanks for the update @ShridharGoel

@ikevin127
Copy link
Contributor

Not overdue, currently awaiting for contributor to open PR based on #46296 (comment).

Copy link

melvin-bot bot commented Aug 19, 2024

@greg-schroeder, @marcaaron, @ShridharGoel, @ikevin127 Eep! 4 days overdue now. Issues have feelings too...

@melvin-bot melvin-bot bot added the Overdue label Aug 19, 2024
@ikevin127

This comment was marked as duplicate.

@melvin-bot melvin-bot bot removed the Overdue label Aug 19, 2024
@ikevin127
Copy link
Contributor

@ShridharGoel Please let us know if you have the capacity to open the PR or if we should move forward with another Contributor for this one. Thanks!

@ShridharGoel
Copy link
Contributor

ShridharGoel commented Aug 21, 2024 via email

@ShridharGoel
Copy link
Contributor

@ikevin127 Maybe you can start the testing from your side till the adhoc builds are available. I need the adhoc builds since I'm unable to build locally.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 21, 2024
@greg-schroeder
Copy link
Contributor

This on staging. Awaiting deploy to prod

@ikevin127
Copy link
Contributor

ikevin127 commented Sep 3, 2024

⚠️ We just got confirmation on Slack that the Deploy Checklist: New Expensify 2024-08-26 which includes the PR of this issue was only deployed to production today in Deploy Checklist: New Expensify 2024-08-28. More context on why this happened can be found in this Slack thread and this Slack comment.

Given the context above, this issue should be on Payment 2024-09-06 according to 4 days ago's production deploy from Deploy Checklist: New Expensify 2024-08-28.

cc @greg-schroeder @marcaaron

@greg-schroeder greg-schroeder changed the title [$250] mWeb - Submit expense - Site crashes sending a distance expense after two scan expenses [HOLD for Payment 2024-09-06] [$250] mWeb - Submit expense - Site crashes sending a distance expense after two scan expenses Sep 4, 2024
@greg-schroeder greg-schroeder added Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review labels Sep 4, 2024
@greg-schroeder
Copy link
Contributor

Updated

@ikevin127
Copy link
Contributor

Regression Test Proposal

  1. Login to staging.new.expensify.com .
  2. Navigate to FAB > Submit expense > Scan.
  3. Upload a receipt and click on Next.
  4. Choose a workspace and submit it.
  5. Navigate to FAB > QAB (Scan IOU).
  6. Upload a receipt and submit it to the same workspace.
  7. Navigate to FAB > Submit expense > Distance.
  8. Enter start and finish locations and click Next.
  9. Select the same workspace you submitted the scan expenses to and click Submit.
  10. Test on different screen sizes / heights (iPhone 4 / 5, Samsung Galaxy S8, etc) and verify that the app doesn't crash when submitting the 3rd Distance request.

Do we agree 👍 or 👎.

@ikevin127
Copy link
Contributor

cc @greg-schroeder

@greg-schroeder
Copy link
Contributor

Sorry, processing

@greg-schroeder
Copy link
Contributor

Paid @ikevin127 , sent new offer to @ShridharGoel. Will pay ASAP once accepted

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. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
No open projects
Status: No status
Development

No branches or pull requests

9 participants