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

Desktop - IOU - The Distance IOU is unable to determine the user's coordinates #38895

Open kbecciv opened 6 months ago

kbecciv commented 6 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.56-0 Reproducible in staging?: y Reproducible in production?: n If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4446001 Issue reported by: Applause - Internal team

Action Performed:

  1. Open NewDot app
  2. Log in to the employee account
  3. Navigate to the WS room
  4. Navigate to Request money -> Distance -> Start
  5. In the Address field, select the "Use current location" option

Expected Result:

When using the "Use current location" option in the Distance IOU, the application must determine the user's coordinates.

Actual Result:

The Distance IOU is unable to determine the user's coordinates.

Workaround:

n/a

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/941e8e76-9943-4a60-8f0b-1d3e04541975

View all open jobs on GitHub

github-actions[bot] commented 6 months ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
melvin-bot[bot] commented 6 months ago

Triggered auto assignment to @roryabraham (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

kbecciv commented 6 months ago

We think that this bug might be related to #wave-collect - Release 1

roryabraham commented 6 months ago

not able to reproduce on dev on main. I suspect this may be a back-end issue

roryabraham commented 6 months ago

not able to reproduce this on staging web either, though I just noticed this was reported on desktop. I'll check there...

roryabraham commented 6 months ago

I got this error on desktop:

image

the link just links to Expensify Help and not a specific page, which isn't very helpful

roryabraham commented 6 months ago

This may be a regression from https://github.com/Expensify/App/pull/38442

roryabraham commented 6 months ago

seeing the same error locally

hayata-suenaga commented 6 months ago

seeing the same error locally

If you haven't manually added the GCP key in your local .env, then it's expected that the current location feature won't work on the local desktop app because of the changes I made

hayata-suenaga commented 6 months ago

but this shouldn't happen on the staging or production. I'll also gonna investigate it

roryabraham commented 6 months ago

Maybe problem is that environment variables set when running electron-builder here aren't made available to the main process at runtime?

roryabraham commented 6 months ago

@hayata-suenaga I'm thinking maybe we can fix this issue with a PR like this: https://github.com/Expensify/App/pull/38953

roryabraham commented 6 months ago

the only problem with that is that I think it might make it available to the renderer process as well, which could mean that a determined actor could read it at runtime. The main process is generally more secure, but still I'm not 100% sure if something set in the Electron main process.env is secure

hayata-suenaga commented 6 months ago

that interesting that webpack doesn't pick up env vars from the runner environment and you have to supply the env file as a command argument 🤔

hayata-suenaga commented 6 months ago

the only problem with that is that I think it might make it available to the renderer process as well, which could mean that a determined actor could read it at runtime. The main process is generally more secure, but still I'm not 100% sure if something set in the Electron main process.env is secure

This issue exists either way because the token needs to be included in the build code.

hayata-suenaga commented 6 months ago

we just wanted to remove the GCP keys from the public App repo

hayata-suenaga commented 6 months ago

the Secrets should be set correctly in the App repo 🤔

roryabraham commented 6 months ago

that interesting that webpack doesn't pick up env vars from the runner environment

I think it makes sense. electron-builder doesn't take all your bash environment variables and inject them into the bundled app - only those included in your .env file (which is consistent with react-native-config)

hayata-suenaga commented 6 months ago

yes that's probably it

we use react-native-config which reads values from the .env file

roryabraham commented 6 months ago

it's actually react-web-config on web and desktop

roryabraham commented 6 months ago

we have a custom version of the webpack plugin to support desktop: https://github.com/Expensify/App/blob/59a71f3d1a34713e6c8213781c423440fb9436f8/config/webpack/webpack.common.js#L122-L130

hayata-suenaga commented 6 months ago

ah I see 😄 wow that's deep

hayata-suenaga commented 6 months ago

@roryabraham we can go with your PR then

roryabraham commented 6 months ago

just going to polish - could use some help writing test / QA steps if you're interested

roryabraham commented 6 months ago

Put the PR in for review, though I'm still working on local testing steps

roryabraham commented 6 months ago

If you haven't manually added the GCP key in your local .env

Where can I find this key?

yuwenmemon commented 6 months ago

Removing the blocker as per the conversation here: https://expensify.slack.com/archives/C01GTK53T8Q/p1711394691005269

melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

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

If no regressions arise, payment will be issued on 2024-04-05. :confetti_ball:

hayata-suenaga commented 6 months ago

No payment is needed for the PR linked above.

The issue hasn't been solved yet. I'll work on this next week.

hayata-suenaga commented 5 months ago

I changed the priority of this task to weekly, but I'll try to get to this this week.

melvin-bot[bot] commented 5 months ago

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

hayata-suenaga commented 5 months ago

The issue is still reproducible. will work on this when I have time.

Screenshot 2024-04-12 at 12 14 04 PM

roryabraham commented 5 months ago

I'm unassigning myself since we don't both need to be assigned

hayata-suenaga commented 5 months ago

I'll work on this after the first release of the QBO project is over 🙇

hayata-suenaga commented 4 months ago

The request to Google geolocation endpoint from Electron is failing. The geolocation API token is not properly injected during build time.

I'll investigate this when I have time after the initial QBO release.

Screenshot 2024-05-08 at 5 26 42 PM
hayata-suenaga commented 4 months ago

I still don't have a time to get around this issue yet. I'll circle back once QBO finishes.

hayata-suenaga commented 3 months ago

still no time to get around this will be back when I have time

hayata-suenaga commented 3 months ago

was putting off this for a while. will take a look during this week 🙇

hayata-suenaga commented 2 months ago

got sick lasst week. catching up with issues.

melvin-bot[bot] commented 1 month ago

This issue has not been updated in over 15 days. @hayata-suenaga eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!