[HOLD for payment 2024-06-06] [$250] Distance Expense - Map is not centered over San Francisco when location settings are turned off

lanitochka17 commented 2 weeks 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.74-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/4561935 Email or phone of affected tester (no customers): applausetester+azadmin@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

Disable the location settings for New Expensify in Chrome
Navigate to Request Money > Distance expense

Expected Result:

The map is centered over San Francisco

Actual Result:

The map is shown over my current city

Workaround:

Unknown

Platforms:

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

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/eae942a7-4eca-453b-b30b-15db9576fd11

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~015240613a85d23a6c
Upwork Job ID: 1790944314356584448
Last Price Increase: 2024-05-16
Automatic offers:

fedirjh | Reviewer | 0
tienifr | Contributor | 0

Issue Owner

Current Issue Owner: @jliexpensify

melvin-bot[bot] commented 2 weeks ago

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

If you find which PR caused the issue/bug, you can reassign it to the person responsible for it.
- If the author is OOO or won’t get online before the daily deploy is due, you are responsible for finding the best fix/path forward. Don’t hesitate to ask for help!
Try to reproduce the issue, if the bug is on production, remove the DeployBlocker label but stay assigned to fix it (or find out which PR broke it to get help from the author).
- You can adjust the urgency of the issue to better represent the gravity of the bug.
- If the issue is super low priority, feel free to un-assign yourself.
- Be careful with PHP warnings, sometimes it is more complex than just adding a null coalescing operator as they might be uncovering some bigger bug.
- If it was a one-off issue that requires no action (for example, Bedrock was down or it is a duplicated issue), you can close it.

Remember rule #2: Never un-assign yourself from a real DeployBlocker unless you are 100% sure someone else is assigned and will take care of it.

melvin-bot[bot] commented 2 weeks ago

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

github-actions[bot] commented 2 weeks 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:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

luacmartins commented 2 weeks ago

This is pretty small and NAB.

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

tienifr commented 2 weeks ago

Proposal

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

The map is shown over my current city

What is the root cause of that problem?

User location is cached in Onyx so when we revoked location permission, the previous location was still there.

When location request failed, we ran a callback to reset to initialLocation which is the default San Francisco but we ealry returned if cachedUserLocation existed:

https://github.com/Expensify/App/blob/0d45c5564188fa63bf1eada357b11e8e3c7e60d9/src/components/MapView/MapView.website.tsx#L67-L69

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

We need to reset location if permission was not granted by removing the cachedUserLocation early return above.

We also need to clear the location stored in Onyx for privacy. We should only save user location as long as they permit it. Otherwise, we might get flickering between the cached location and the current or initial location.

if (!initialState) {
    return;
}
UserLocation.clear();
setCurrentPosition(initialState.location);

What alternative solutions did you explore? (Optional)

We can optionally check for PERMISSION_DENIED error code to only clear the cache for this specific case when user denied permission.

https://github.com/Expensify/App/blob/c5f84b6a5c744e020fbb467f085931ce81534c37/src/libs/getCurrentPosition/getCurrentPosition.types.ts#L21-L23

There's another issue that when there's cached location and the permission was reset, while the permission prompt was still open, the map would show the cached location instead of default location (San Francisco) because userLocation is initialized with cachedLocation while it should be the default location initialState.location:

https://github.com/Expensify/App/blob/0d45c5564188fa63bf1eada357b11e8e3c7e60d9/src/components/MapView/MapView.website.tsx#L53

szhaqypbek commented 2 weeks ago

Proposal

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

The map is shown over my current city when user geolocation settings are disabled

What is the root cause of that problem?

setCurrentPositionToInitialState callback returns when the user's location is already cached in Onyx. https://github.com/Expensify/App/blob/0d45c5564188fa63bf1eada357b11e8e3c7e60d9/src/components/MapView/MapView.website.tsx#L67-L69

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

We should check for an error code returned from the getCurrentPosition callback and set a default location when error.code equals GeolocationErrorCode.PERMISSION_DENIED

if (error.code !== GeolocationErrorCode.PERMISSION_DENIED && (cachedUserLocation || !initialState)) {
  return;
}

What alternative solutions did you explore? (Optional)

Screenshots/Videos

https://github.com/Expensify/App/assets/16764625/eb0b9737-f1da-4300-afc5-ded964ad85bd

tienifr commented 2 weeks ago

Proposal updated to add optional solution and detailed explaination.

fedirjh commented 2 weeks ago

Although both proposals are valid, I am inclined to favor the idea of completely removing the cached data when the location permission is revoked.

Let's move forward with @tienifr's proposal.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 2 weeks ago

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

fedirjh commented 1 week ago

Update: Awaiting @luacmartins to sign off the proposal

melvin-bot[bot] commented 1 week ago

📣 @fedirjh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 week ago

📣 @tienifr 🎉 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 📖

slafortune commented 1 week ago

There is a PR draft in review. I'll be out until June 4th - so adding another BZ in the meantime. I'll take this back on the 4th if needed.

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 3 days ago

⚠️ 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.

jliexpensify commented 3 days ago

@fedirjh any regressions here?

fedirjh commented 2 days ago

any regressions here?

There was a regression in https://github.com/Expensify/App/issues/42752 , and it's already fixed by @tienifr.

melvin-bot[bot] commented 2 days ago

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

melvin-bot[bot] commented 2 days ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.77-11 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-06-06. :confetti_ball:

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

@fedirjh requires payment automatic offer (Reviewer)
@tienifr requires payment automatic offer (Contributor)

melvin-bot[bot] commented 2 days ago

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

[ ] [@fedirjh] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@fedirjh] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] [@fedirjh] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[ ] [@fedirjh] Determine if we should create a regression test for this bug.
[ ] [@fedirjh] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

jliexpensify commented 2 days ago

Ok, is this a correct Summary?

C: @tienifr $250
C+: @fedirjh $250 - please accept the offer!

Upwork job

Just waiting on the checklist (if applicable)

Expensify / App