[WAITING ON FEDI - CHECKLIST] Distance Expense - Map is not centered over San Francisco when location settings are turned off

[lanitochka17]

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.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): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

1. Disable the location settings for New Expensify in Chrome
2. 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
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6482032_1715806856342.map_San_Fransico.mp4

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]

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

1. 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!
2. 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]

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

[github-actions]

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

[luacmartins]

This is pretty small and NAB.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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]

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:

App/src/components/MapView/MapView.website.tsx

Lines 67 to 69 in 0d45c55

	if (cachedUserLocation || !initialState) {
	return;
	}

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.

App/src/libs/getCurrentPosition/getCurrentPosition.types.ts

Lines 21 to 23 in c5f84b6

	PERMISSION_DENIED: typeof GeolocationErrorCode.PERMISSION_DENIED;
	POSITION_UNAVAILABLE: typeof GeolocationErrorCode.POSITION_UNAVAILABLE;
	TIMEOUT: typeof GeolocationErrorCode.TIMEOUT;

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:

App/src/components/MapView/MapView.website.tsx

Line 53 in 0d45c55

const [currentPosition, setCurrentPosition] = useState(cachedUserLocation);

[szhaqypbek]

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.

App/src/components/MapView/MapView.website.tsx

Lines 67 to 69 in 0d45c55

	if (cachedUserLocation || !initialState) {
	return;
	}

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

Screen.Recording.2024-05-16.at.09.mp4

[tienifr]

Proposal updated to add optional solution and detailed explaination.

[fedirjh]

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]

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

[fedirjh]

Update: Awaiting @luacmartins to sign off the proposal

[jliexpensify]

@fedirjh any regressions here?

[fedirjh]

any regressions here?

There was a regression in #42752 , and it's already fixed by @tienifr.

[melvin-bot]

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

[melvin-bot]

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

• Clear user location cache when they denied location permission #42509
• [CP Staging] fix: pending map in IOU confirmation step #42773

If no regressions arise, payment will be issued on 2024-06-06. 🎊

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]

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]

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)

[melvin-bot]

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

• [CP Staging] fix: pending map in IOU confirmation step #42773

If no regressions arise, payment will be issued on 2024-06-11. 🎊

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]

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]

Not overdue. Bump @fedirjh for the checklist.

[jliexpensify]

Paid and job closed. Bump @fedirjh for the checklist!

[fedirjh]

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

@jliexpensify I don't think we need any regression test for this one. It was already caught with this regression test: https://expensify.testrail.io/index.php?/tests/view/4561935. I guess we should just close.

BugZero Checklist:

• Link to the PR: Fix Mapbox open to ocean on the first render #41480
• Link to comment: Fix Mapbox open to ocean on the first render #41480 (comment)
• Link to discussion: N/A
• Determine if we should create a regression test for this bug: N/a

[luacmartins]

Agreed. I think we're all good to close here. Please reopen if needed.