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

[Due for payment 2025-03-17] [$500] Desktop - Scan receipts - There is a delay after clicking the submit expense button #52668

Open
1 of 8 tasks
IuliiaHerets opened this issue Nov 16, 2024 · 151 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

@IuliiaHerets
Copy link

IuliiaHerets commented Nov 16, 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: v9.0.63-1
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/5231836&group_by=cases:section_id&group_id=294998&group_order=asc
Issue reported by: Applause Internal Team

Action Performed:

  1. Navigate to the workspace chat as the employee
  2. Select the option to Submit expense > Scan receipt
  3. Select the option to upload a receipt
  4. Upload a receipt file
  5. Click the "Submit expense" button

Expected Result:

The submitting expense flow using scanning option is smooth

Actual Result:

There is a delay after clicking the submit expense button

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6666749_1731706218416.scan_receipt.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021858847049782861721
  • Upwork Job ID: 1858847049782861721
  • Last Price Increase: 2025-01-03
  • Automatic offers:
    • wildan-m | Contributor | 106254364
Issue OwnerCurrent Issue Owner: @zanyrenney
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 16, 2024
Copy link

melvin-bot bot commented Nov 16, 2024

Triggered auto assignment to @zanyrenney (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.

@daledah
Copy link
Contributor

daledah commented Nov 18, 2024

Proposal

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

There is a delay after clicking the submit expense button

What is the root cause of that problem?

In

App/desktop/main.ts

Lines 24 to 27 in 63c8dd2

// Setup google api key in process environment, we are setting it this way intentionally. It is required by the
// geolocation api (window.navigator.geolocation.getCurrentPosition) to work on desktop.
// Source: https://github.com/electron/electron/blob/98cd16d336f512406eee3565be1cead86514db7b/docs/api/environment-variables.md#google_api_key
process.env.GOOGLE_API_KEY = CONFIG.GCP_GEOLOCATION_API_KEY;

it mentioned that we need to provide the GCP_GEOLOCATION_API_KEY, but it's not available on desktop dev env or it can be expired on staging/production

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

This issue happens if we don't provide GCP_GEOLOCATION_API_KEY, especially on dev env where we don't have it

If GCP_GEOLOCATION_API_KEY is undefined on desktop, we can skip navigator.geolocation.getCurrentPosition

on desktop.ts

CONFIG.GCP_GEOLOCATION_API_KEY;

const getCurrentPosition: GetCurrentPosition = (success, error, options) => {
    if (navigator === undefined || !('geolocation' in navigator) || !CONFIG.GCP_GEOLOCATION_API_KEY) {
...
return;
}

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Nov 19, 2024
@zanyrenney
Copy link
Contributor

Nice, looks like we also already have a proposal.

@melvin-bot melvin-bot bot removed the Overdue label Nov 19, 2024
@zanyrenney
Copy link
Contributor

Moving ahead here.

@zanyrenney zanyrenney added the External Added to denote the issue can be worked on by a contributor label Nov 19, 2024
Copy link

melvin-bot bot commented Nov 19, 2024

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

@melvin-bot melvin-bot bot changed the title Desktop - Scan receipts - There is a delay after clicking the submit expense button [$250] Desktop - Scan receipts - There is a delay after clicking the submit expense button Nov 19, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 19, 2024
Copy link

melvin-bot bot commented Nov 19, 2024

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

@dipanshujindal1992
Copy link

dipanshujindal1992 commented Nov 19, 2024

Hi

I checked that and i find the root cause. Basically we are calling the api and api takes time to write the data especially while we uploaidng the image. In order to prevent multiple clicks, they are using the

// Don't let the form be submitted multiple times while the navigator is waiting to take the user to a different page
            if (formHasBeenSubmitted.current) {
                return;
            }

            formHasBeenSubmitted.current = true;

### The Solution is

We can display the ActivityIndicator until api success or fails on Press of SubmitExpenses.

Also, I see lot of warnings(94) in the code that needs to be addressed.

Contributor details
Your Expensify account email: dipanshujindal1993@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/dipanshuj

simulator_screenshot_7D329B78-2193-4713-8AFD-D4C19EF929FD

Copy link

melvin-bot bot commented Nov 19, 2024

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

@daledah
Copy link
Contributor

daledah commented Nov 21, 2024

@c3024 What do you think about my proposal? Thanks

@dipanshujindal1992
Copy link

Hi @c3024
Did you get the chance to review my proposals ?

@melvin-bot melvin-bot bot added the Overdue label Nov 21, 2024
Copy link

melvin-bot bot commented Nov 21, 2024

📣 @dipanshujindal1992! 📣
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:

  1. Make sure you've read and understood the contributing guidelines.
  2. 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.
  3. 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.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@c3024
Copy link
Contributor

c3024 commented Nov 21, 2024

Thanks for the tags.

@daledah , I will update soon.

@dipanshujindal1992 , Please check out the contributing guidelines. Only proposals in the format given there will be reviewed.

@melvin-bot melvin-bot bot removed the Overdue label Nov 21, 2024
@dipanshujindal1992
Copy link

Hi @c3024

I uploaded the content as your format. May i know what is missing in my format?

Contributor details
Your Expensify account email: dipanshujindal1993@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/dipanshuj

Copy link

melvin-bot bot commented Nov 21, 2024

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

@c3024
Copy link
Contributor

c3024 commented Nov 23, 2024

@daledah , getLocationPermission returns value as GRANTED but still fetching the coordiantes gets timed out on desktop build. This is working well with Chrome. Config value for GCP_GEOLOCATION_API_KEY is empty even for Chrome. If we use this check you suggested

CONFIG.GCP_GEOLOCATION_API_KEY;

const getCurrentPosition: GetCurrentPosition = (success, error, options) => {
    if (navigator === undefined || !('geolocation' in navigator) || !CONFIG.GCP_GEOLOCATION_API_KEY) {
...
return;
}

on desktop, we will skip fetching coordinates everytime. I think it needs to be found if the check

navigator.permissions.query({name: 'geolocation'})

in getLocationPermission is correctly returning the result.

@daledah
Copy link
Contributor

daledah commented Nov 25, 2024

@c3024 getCurrentPosition works well on web plattform

Ref: https://developer.mozilla.org/en-US/docs/Web/API/Geolocation/getCurrentPosition#:~:text=This%20feature%20is%20available%20only%20in%20secure%20contexts%20(HTTPS)%2C%20in%20some%20or%20all%20supporting%20browsers.

But on desktop, we need to add GOOGLE_API_KEY as we mentioned here

App/desktop/main.ts

Lines 24 to 27 in 63c8dd2

// Setup google api key in process environment, we are setting it this way intentionally. It is required by the
// geolocation api (window.navigator.geolocation.getCurrentPosition) to work on desktop.
// Source: https://github.com/electron/electron/blob/98cd16d336f512406eee3565be1cead86514db7b/docs/api/environment-variables.md#google_api_key
process.env.GOOGLE_API_KEY = CONFIG.GCP_GEOLOCATION_API_KEY;

that why I think we should add CONFIG.GCP_GEOLOCATION_API_KEY check for desktop only to skip fetching coordinates

@melvin-bot melvin-bot bot added the Overdue label Nov 25, 2024
@c3024
Copy link
Contributor

c3024 commented Nov 25, 2024

There may be an issue with the GCP_GEOLOCATION_API_KEY in the secrets, as the request consistently times out. Could the internal engineer please verify and confirm?

🎀 👀 🎀

@melvin-bot melvin-bot bot removed the Overdue label Nov 25, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 24, 2025
Copy link

melvin-bot bot commented Feb 24, 2025

📣 @wildan-m 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot removed the Overdue label Feb 24, 2025
@wildan-m
Copy link
Contributor

@dannymcclain @dubielzyk-expensify @shawnborton
Can I get the text confirmation for the second pop up. Is the text ok? this will only show for desktop
Image

Complete flow

Kapture.2025-02-11.at.17.16.11.mp4

@dubielzyk-expensify
Copy link
Contributor

Why is the loading spinner so long? Is this expected or are we testing this on slower networks?

As for the screen where location permission already has been denied, we should use this copy:

Image

After they tap Settings, I think we should actually stay on that dialog until we've detected that they've turned on Location permission. Do we really need to relaunch the app to make use of the new permission? On macOS I've noticed that location permission seems to not do that for other apps, but for things like screen recording permission you have to trigger a relaunch.

The reason why I'm hesitant with a relaunch is that we'll break their flow and discard their work so far, right? So I'd rather not lose the image and things they've done unless it's crucial for us to have that information before proceeding?

@wildan-m
Copy link
Contributor

@dubielzyk-expensify

Why is the loading spinner so long? Is this expected or are we testing this on slower networks?

This is the specific case when we disabled the location service, we currently set the timeout to get location at 15 seconds. Can we reduce the time? unfortunately we shouldn't. When we invoke navigator.geolocation.getCurrentPosition, it may complete quickly or take up to tens seconds. If we were to shorten the timeout to, let's say, 5 seconds, we might never get location coordinate, since the timeout will reached before it. In summary, the delay is unavoidable.

As for the screen where location permission already has been denied, we should use this copy:

That screen will remain, but we add additional screen for desktop after user tapping Setting button.

Do we really need to relaunch the app to make use of the new permission?

After re-testing, the location can be fetched directly without needing to re-launch. The initial reason I suggested this was because previously it did not take effect immediately after toggling the location service. I will test the solution without re-launching and update you on whether it is still necessary.

@dannymcclain
Copy link
Contributor

Sounds good—I agree with all of Jon's comments.

@melvin-bot melvin-bot bot added the Overdue label Feb 26, 2025
@shawnborton
Copy link
Contributor

Yup, agree as well - glad we don't need to force the user to relaunch the app, that felt weird!

@wildan-m
Copy link
Contributor

After they tap Settings, I think we should actually stay on that dialog until we've detected that they've turned on Location permission.

@dubielzyk-expensify when the setting enabled, there is no OS event that directly tells the app that some settings has been modified. Our options are:

  1. Pooling, check location for every x second when the modal shown
  2. Check only when we re-focus the app
  3. Provide / change Settings button to Check Location, that will manually check the permission.

@luacmartins @c3024 any suggestion which way to choose?

@dubielzyk-expensify
Copy link
Contributor

I can't speak to technicalities, but I prefer 1 or 2 over a relaunch at least. If 2 is reliable, then that feels pretty reaasonable to me.

I feel like option 3 is a foreign concept to a user, but maybe with the right copy like "Retry" or something. Still prefer 1 and 2, but I'll let the technical part for others.

@c3024
Copy link
Contributor

c3024 commented Feb 27, 2025

Yea, both (1) and (2) LGTM.

I think we can use a combination of them.

  1. For most cases, the polling will find that the location has been enabled before the user returns to the app.
  2. If, for some reason, the polling until that time could not establish this, we can check for it immediately when the user refocuses the app.

@melvin-bot melvin-bot bot removed the Overdue label Feb 27, 2025
@luacmartins
Copy link
Contributor

I'd prefer 2 to avoid constant polling.

@wildan-m
Copy link
Contributor

wildan-m commented Mar 1, 2025

@luacmartins when we call requestMoney, we save the GPS coordinates of the receipt. However, I noticed that there was no location information in the response for the transaction. Could you clarify what that location is used for and where it is stored?

@c3024
Copy link
Contributor

c3024 commented Mar 1, 2025

It is used to guess the currency and timezone of the user submitting the receipt to improve the accuracy of currency detection. The backend uses it for this purpose but does not return it in the response.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Mar 3, 2025
@wildan-m
Copy link
Contributor

wildan-m commented Mar 3, 2025

It is used to guess the currency and timezone of the user submitting the receipt to improve the accuracy of currency detection. The backend uses it for this purpose but does not return it in the response.

@c3024 Thanks for clarification, I guess only dev can fully verify the result then. The PR is ready #57660. Thanks!

@luacmartins
Copy link
Contributor

What @c3024 said is correct. Thanks for opening the PR for review @wildan-m. @c3024 let's get this one reviewed once you're online.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 10, 2025
@melvin-bot melvin-bot bot changed the title [$500] Desktop - Scan receipts - There is a delay after clicking the submit expense button [Due for payment 2025-03-17] [$500] Desktop - Scan receipts - There is a delay after clicking the submit expense button Mar 10, 2025
Copy link

melvin-bot bot commented Mar 10, 2025

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

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 10, 2025
Copy link

melvin-bot bot commented Mar 10, 2025

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.1.10-6 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 2025-03-17. 🎊

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

Copy link

melvin-bot bot commented Mar 10, 2025

@c3024 @zanyrenney @c3024 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

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
Status: Hold for Payment
Development

No branches or pull requests