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 2023-12-06] [DISTANCE] [$500] HIGH: Automatically pan initial map based on current location #22704

Closed
neil-marcellini opened this issue Jul 11, 2023 · 39 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Distance Wave5-free-submitters Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@neil-marcellini
Copy link
Contributor

neil-marcellini commented Jul 11, 2023

Please follow the plan in the design doc. I have pasted the plan below for external viewers so some of the links might not work. Also, please note that contrary to the original plan below, we have already implemented the "Use current location" button, so please try to reuse as much of that code as possible.

Automatic user location access

The first time a user opens this page we will prompt them to enable location services and then show a map centered on their current location. We’re not going to get the location using the mapping libraries because rnmapbox/maps doesn’t provide a way to access the location permissions. For web, react-map-gl and the underlying Mapbox gl js library don’t provide a way to request the user’s location without forcing them to press a button.

To handle geolocation we will need to platform specific files because react-native-geolocation doesn’t support web. We will create a location folder with platform specific implementations for all the functions handling the user location. At the moment we only need getCurrentPosition.

On native at the top of the file we will set up the configuration via Geolocation.setRNConfiguration. In the config we’ll set skipPermissionRequests: false since we’ll be using this function to request permission, authorizationLevel: ‘whenInUse’ because we only need the location when the app is open, and locationProvider: ‘auto’.

getCurrentPosition(success, error, config) will call Geolocation.getCurrentPosition passing the callbacks config through. From the App side we’ll set up a default config with the timeout and the maximumAge set to one minute because it seems like a reasonable maximum time to wait for your location and it’s unlikely that a cached location will be out of date within one minute.

On web getCurrentPosition(success, error, config) will first check if navigator.geolocation exists. If not then we will call the error call back with a GeolocationPositionError object with code 2, POSITION_UNAVAILABLE. Otherwise, we’ll call navigator.geolocation.getCurrentPosition passing through the params.

After calling getCurrentPosition in the ExpensifyMap, in the success callback we will center the map on the user’s location. The error callback will do nothing for this location request, because if we fail to get the user’s location we’ll continue displaying the default map area of San Francisco. We can prompt the user to enable location permission when they ask for it more explicitly on the waypoint editor page.

On native to center the map we’ll use camera.flyTo([lon, lat]); on the Camera ref. The Camera component will be a child of the MapView as shown in this example.

On web we will call map.flyTo({center: [lon, lat]}); where the map comes from the useMap hook.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b6a985f453bad073
  • Upwork Job ID: 1711808933186453504
  • Last Price Increase: 2023-10-24
@melvin-bot
Copy link

melvin-bot bot commented Jul 17, 2023

Eep! 4 days overdue now. Issues have feelings too...

@melvin-bot melvin-bot bot added the Overdue label Jul 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 19, 2023

6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@melvin-bot
Copy link

melvin-bot bot commented Aug 21, 2023

This issue has not been updated in over 15 days. 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!

@luacmartins
Copy link
Contributor

On hold

@neil-marcellini neil-marcellini changed the title [HOLD 22703] Automatic user location access for distance requests Automatic user location access for distance requests Aug 21, 2023
@neil-marcellini neil-marcellini added Daily KSv2 and removed Monthly KSv2 labels Aug 21, 2023
@neil-marcellini
Copy link
Contributor Author

Carlos is focused on wave 3/4 so we're going to look for another volunteer.

@thienlnam
Copy link
Contributor

Expensify/react-native-x-geolocation#8

Should be the last PR for everything to be needed for the App PR

@melvin-bot melvin-bot bot added the Overdue label Aug 28, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 28, 2023

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

@thienlnam
Copy link
Contributor

App PR is in progress and tracked in a different issue - all is done here regarding the new library

@melvin-bot melvin-bot bot removed the Overdue label Aug 28, 2023
@neil-marcellini
Copy link
Contributor Author

I realized that we still haven't implemented this in App, so I'm reopening it and making it external. I'll paste the plan from the design doc into the issue description.

@neil-marcellini neil-marcellini added the External Added to denote the issue can be worked on by a contributor label Oct 10, 2023
@melvin-bot melvin-bot bot changed the title Automatic user location access for distance requests [$500] Automatic user location access for distance requests Oct 10, 2023
@dylanexpensify dylanexpensify changed the title [$500] Automatically pan initial map based on current location [DISTANCE] [$500] HIGH: Automatically pan initial map based on current location Nov 7, 2023
@dylanexpensify dylanexpensify added the Distance Wave5-free-submitters label Nov 7, 2023
@stephanieelliott
Copy link
Contributor

PR changes have been applied, just awaiting approving review.

@stephanieelliott
Copy link
Contributor

@MaciejSWM, heads up this one is blocked on you resolving conflicts on the PR!

@stephanieelliott
Copy link
Contributor

PR was deployed to staging a few hours ago 🚀

Copy link

melvin-bot bot commented Nov 28, 2023

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 29, 2023
@melvin-bot melvin-bot bot changed the title [DISTANCE] [$500] HIGH: Automatically pan initial map based on current location [HOLD for payment 2023-12-06] [DISTANCE] [$500] HIGH: Automatically pan initial map based on current location Nov 29, 2023
Copy link

melvin-bot bot commented Nov 29, 2023

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

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Nov 29, 2023
Copy link

melvin-bot bot commented Nov 29, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.4-3 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 2023-12-06. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

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

  • @mananjadhav does not require payment (Eligible for Manual Requests)
  • @MaciejSWM does not require payment (Contractor)

Copy link

melvin-bot bot commented Nov 29, 2023

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

  • [@mananjadhav] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@stephanieelliott] 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 Overdue and removed Weekly KSv2 Daily KSv2 labels Dec 5, 2023
@stephanieelliott
Copy link
Contributor

Summarizing payment on this issue:

  • Contributor: @MaciejSWM, no payment required
  • Contributor+: $500 - HOLD on BZ checklist, @mananjadhav can you please complete?
    Upwork job is here

@melvin-bot melvin-bot bot removed the Overdue label Dec 8, 2023
@mananjadhav
Copy link
Collaborator

@stephanieelliott This was more of a feature request than a bug.

But I think we should add a regression test. The test steps from the PR are good to be added the regression tests.

@JmillsExpensify
Copy link

$500 payment approved for @mananjadhav based on summary above.

@melvin-bot melvin-bot bot added the Overdue label Dec 11, 2023
@JmillsExpensify
Copy link

@mananjadhav can you please paste the regression/TestRail steps as a comment in this issue?

@thienlnam
Copy link
Contributor

Pending regression steps from C+

@melvin-bot melvin-bot bot removed the Overdue label Dec 12, 2023
@mananjadhav
Copy link
Collaborator

mananjadhav commented Dec 12, 2023

We have three scenarios for the regression test steps. @JmillsExpensify @stephanieelliott @thienlnam We can use the following scenarios to add to Testrail.

Regression Test Steps

Location Permission is not set

  1. Have the location access reset before you access the app.
  2. Login to the app
  3. Click on FAB -> Request Money -> Distance.
  4. Browser should ask if you want to grant location access. Map should stay in pending state until you choose something.
  5. Grant the permission. Map should navigate straight to your current location within 1-2 seconds.
  6. Repeat steps 1 to 4, but this time, block the location access. The map should redirect you to default San Francisco location.

Location Permission is enabled

  1. Load the application with the location access enabled.
  2. Start the request money flow by clicking FAB -> Request Money -> Distance.
  3. It should take you directly to your current location.
  4. Update the location in the Dev console by setting:
Onyx.set(ONYXKEYS.USER_LOCATION, { longitude: 12, latitude: 12 })
  1. Repeat the steps 1 and 2. It should change the map location to your current location.
  2. Repeat steps 4 and 5, and before it can change to your current location, drag the map. This should keep you at [12, 12] coords and should stop the map from auto-panning to your current location.

Location Permission is disabled

  1. Load the application with the location access enabled.
  2. Start the request money flow by clicking FAB -> Request Money -> Distance.
  3. The map should redirect you to default San Francisco location.
  4. Refresh the page. You should land straight in the San Francisco location.
  5. Enable the location and refresh the page to cache the current location in ONYX.
  6. Then disable the location and refresh the page.
  7. You should land in this cached location instead of the default San Francisco location.

@stephanieelliott
Copy link
Contributor

Thank you -- issue for regression test here: https://github.com/Expensify/Expensify/issues/349072

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 Daily KSv2 Distance Wave5-free-submitters Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
No open projects
Development

No branches or pull requests

8 participants