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

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

Closed
1 of 6 tasks
kbecciv opened this issue Mar 24, 2024 · 42 comments
Closed
1 of 6 tasks

Comments

@kbecciv
Copy link

kbecciv commented Mar 24, 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.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?

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6424107_1711180240019.Recording__1464.mp4

View all open jobs on GitHub

@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Mar 24, 2024
Copy link
Contributor

👋 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.

Copy link

melvin-bot bot commented Mar 24, 2024

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

@kbecciv
Copy link
Author

kbecciv commented Mar 24, 2024

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

@roryabraham
Copy link
Contributor

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

@roryabraham
Copy link
Contributor

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

@roryabraham
Copy link
Contributor

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
Copy link
Contributor

This may be a regression from #38442

@roryabraham
Copy link
Contributor

seeing the same error locally

@hayata-suenaga
Copy link
Contributor

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
Copy link
Contributor

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

@roryabraham
Copy link
Contributor

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

@roryabraham
Copy link
Contributor

@hayata-suenaga I'm thinking maybe we can fix this issue with a PR like this: #38953

@roryabraham
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@hayata-suenaga
Copy link
Contributor

@roryabraham
Copy link
Contributor

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
Copy link
Contributor

@roryabraham
Copy link
Contributor

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

@roryabraham
Copy link
Contributor

we have a custom version of the webpack plugin to support desktop:

new DefinePlugin({
...(platform === 'desktop' ? {} : {process: {env: {}}}),
__REACT_WEB_CONFIG__: JSON.stringify(dotenv.config({path: envFile}).parsed),
// React Native JavaScript environment requires the global __DEV__ variable to be accessible.
// react-native-render-html uses variable to log exclusively during development.
// See https://reactnative.dev/docs/javascript-environment
__DEV__: /staging|prod|adhoc/.test(envFile) === false,
}),

@hayata-suenaga
Copy link
Contributor

ah I see 😄 wow that's deep

@hayata-suenaga
Copy link
Contributor

@roryabraham we can go with your PR then

@roryabraham
Copy link
Contributor

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

@roryabraham roryabraham removed their assignment Apr 15, 2024
@roryabraham
Copy link
Contributor

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

@hayata-suenaga
Copy link
Contributor

hayata-suenaga commented Apr 29, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Apr 29, 2024
@melvin-bot melvin-bot bot added the Overdue label May 8, 2024
@hayata-suenaga
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label May 9, 2024
@melvin-bot melvin-bot bot added the Overdue label May 17, 2024
@hayata-suenaga
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label May 20, 2024
@melvin-bot melvin-bot bot added the Overdue label May 29, 2024
@hayata-suenaga
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Jun 5, 2024
@melvin-bot melvin-bot bot added the Overdue label Jun 13, 2024
@hayata-suenaga
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Jun 24, 2024
@melvin-bot melvin-bot bot added the Overdue label Jul 2, 2024
@hayata-suenaga
Copy link
Contributor

got sick lasst week. catching up with issues.

@melvin-bot melvin-bot bot removed the Overdue label Jul 8, 2024
@melvin-bot melvin-bot bot added the Overdue label Jul 16, 2024
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Jul 31, 2024
Copy link

melvin-bot bot commented Jul 31, 2024

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!

Copy link

melvin-bot bot commented Oct 21, 2024

@kbecciv, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants