[HOLD for payment 2024-07-10][$250] 'You location' button is displayed on distance request thumbnail

m-natarajan commented 3 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.80-16 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: n/a Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause internal team Slack conversation:

Action Performed:

Navigate to a workspace chat
Make a distance request
Open the IOU report

Expected Result:

There shouldn't be 'Your location' button on the thumbnail

Actual Result:

There is 'Your location' button on the thumbnail

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

https://github.com/Expensify/App/assets/38435837/ac1d9c13-1a10-4ddf-a0a6-a2c040227a94

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~015e9f59317ae04047
Upwork Job ID: 1800208798311565511
Last Price Increase: 2024-06-10
Automatic offers:

hungvu193 | Reviewer | 102701269

Issue Owner

Current Issue Owner: @lschurr

melvin-bot[bot] commented 3 months ago

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

m-natarajan commented 3 months ago

@lschurr FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

m-natarajan commented 3 months ago

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

Krishna2323 commented 3 months ago

Proposal

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

'You location' button is displayed on distance request thumbnail

What is the root cause of that problem?

The button is always displayed. https://github.com/Expensify/App/blob/4176e95f4fa0867a56092af23193c6e2c8f4e23e/src/components/MapView/MapView.tsx#L241-L257 https://github.com/Expensify/App/blob/4176e95f4fa0867a56092af23193c6e2c8f4e23e/src/components/MapView/MapView.website.tsx#L255-L271

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

We can use the interactive prop to hide the button for both web and native.

Bug 2: User location (blue dot) is also shown in the thumbnail (ConfirmedRoute). If we also want to hide the user location, we can use interactive prop or create a new prop for hiding that. https://github.com/Expensify/App/blob/60442419ab6434f1f22729399637c8c67c0167a3/src/components/MapView/MapView.website.tsx#L231-L237

Bug 3: On confirmation page (also in money request preview), the map focuses on the users current location first and then animates to the waypoints. This happens because currentPosition initial state is set to cachedUserLocation. We set it to the waypoints but until then the map is already loaded. To resolve this, we should use shouldPanMapToCurrentPosition variable to set the initial state of currentPosition accordingly. https://github.com/Expensify/App/blob/60442419ab6434f1f22729399637c8c67c0167a3/src/components/MapView/MapView.website.tsx#L60 https://github.com/Expensify/App/blob/60442419ab6434f1f22729399637c8c67c0167a3/src/components/MapView/MapView.website.tsx#L95-L97

const shouldPanMapToCurrentPosition = useCallback(() => !userInteractedWithMap && (!waypoints || waypoints.length === 0), [userInteractedWithMap, waypoints]);
const [currentPosition, setCurrentPosition] = useState(shouldPanMapToCurrentPosition() && cachedUserLocation ? cachedUserLocation : initialLocation);

What alternative solutions did you explore? (Optional)

We can also introduce a new prop for hiding the button and pass that prop MoneyRequestView to hide the button, similar to interactive prop.
We can disable the press event on the button and style the cursor according to the prop.

melvin-bot[bot] commented 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~015e9f59317ae04047

melvin-bot[bot] commented 3 months ago

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

truph01 commented 3 months ago

Proposal

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

'You location' button is displayed on distance request thumbnail

What is the root cause of that problem?
We always display 'Your location' button: https://github.com/Expensify/App/blob/e66ae37d8cf4c4ea2a8c18bae48763fd0a734982/src/components/MapView/MapView.website.tsx#L256
And we have a similar logic in MapView.native.tsx as well.

What changes do you think we should make in order to solve the problem?
In this logic: https://github.com/Expensify/App/blob/e66ae37d8cf4c4ea2a8c18bae48763fd0a734982/src/components/ReportActionItem/ReportActionItemImage.tsx#L80-L83 We integrate a map view component to preview the distance map while awaiting the backend to provide the map preview in image format for the IOU report. The ConfirmedRoute component is intended to match the output that the backend will eventually deliver. However, in the staging environment, there are significant discrepancies between the behavior of the ConfirmedRoute component and the backend's expected output:
1. The ConfirmedRoute display the 'Your location' button in the top right (main bug).
2. The blue dot indicates that it is the current user location.
3. When the ConfirmedRoute loaded, it focus on the current user location and then, it animates to the routes. But the expected is, ConfirmedRoute always display the routes instead of the current user location initially.
Based on the above, I propose a solution following by the below steps:
1. Create a new param in MapView (shouldDisplayUserLocation with default value is false for example). We can consider using the existed param, interactive.

Then, we just display this component: https://github.com/Expensify/App/blob/e66ae37d8cf4c4ea2a8c18bae48763fd0a734982/src/components/MapView/MapView.website.tsx#L255 if shouldDisplayUserLocation is true.
Then, just display this component: https://github.com/Expensify/App/blob/e66ae37d8cf4c4ea2a8c18bae48763fd0a734982/src/components/MapView/MapView.website.tsx#L231 if shouldDisplayUserLocation is true.
Then, an additional condition && shouldDisplayUserLocation to this condition: https://github.com/Expensify/App/blob/e66ae37d8cf4c4ea2a8c18bae48763fd0a734982/src/components/MapView/MapView.website.tsx#L240

Then, add a new variable in MapView.website.tsx:

const initialViewState = useMemo(() => {
        if(!shouldDisplayUserLocation){
            if(!waypoints){
                return undefined
            }
            const {northEast, southWest} = utils.getBounds(
                waypoints.map((waypoint) => waypoint.coordinate),
                directionCoordinates,
            );
            return {
                zoom: initialState.zoom,
                bounds: [northEast, southWest]
            }
        }
        else {
            return {
                longitude: currentPosition?.longitude,
                latitude: currentPosition?.latitude,
                zoom: initialState.zoom,
            };
        }
    }, [waypoints, directionCoordinates, shouldDisplayUserLocation]);

and use it in this logic instead: https://github.com/Expensify/App/blob/e66ae37d8cf4c4ea2a8c18bae48763fd0a734982/src/components/MapView/MapView.website.tsx#L222

Finally, use shouldDisplayUserLocation={true} in where want to display the 'Your location' button.
What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

Krishna2323 commented 3 months ago

Proposal updated

Added solutions for extra bugs mentioned here.

hungvu193 commented 3 months ago

Reviewing shortly

hungvu193 commented 3 months ago

3. Then, just display this component: https://github.com/Expensify/App/blob/e66ae37d8cf4c4ea2a8c18bae48763fd0a734982/src/components/MapView/MapView.website.tsx#L231
if `shouldDisplayUserLocation` is `true`.

@truph01 Can you explain why we should only show markers (routes) when shouldDisplayUserLocation is true? I believe we still want to show routes on the thumbnail.

truph01 commented 3 months ago

@hungvu193 That marker is to display the blue dot, not the routes: https://github.com/Expensify/App/blob/e66ae37d8cf4c4ea2a8c18bae48763fd0a734982/src/components/MapView/MapView.website.tsx#L231-L237

So I mean that we just display the blue dot if shouldDisplayUserLocation is true. The waypoints is still as it is.

hungvu193 commented 3 months ago

That marker is to display the blue dot, not the routes:

Oops, my bad I was chasing wrong mentioned code 🥶 .

Thanks for proposals everyone. I believe we want to hide that blue dot along with You location button. (cc @lschurr for confirming this one) In this case, I'd go with @truph01 's proposal here. 🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @mountiny, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

Krishna2323 commented 3 months ago

@hungvu193, what do you think about my proposal? My proposal covers all three issues and the other issues are not part of original issues, so I don't think the proposal should be selected based on that. Hiding the blue dot will be same as hiding the your location button so I don't think the selected proposal is different from mine.

hungvu193 commented 3 months ago

Hey @Krishna2323, I chose @truph01 proposal because it has more detail about RCA and solution. I don't think

We can use the interactive prop to hide the button for both web and native.

or

We can also introduce a new prop for hiding the button and pass that prop MoneyRequestView to hide the button, similar to interactive prop.

is detail enough. I know that contributors are always under pressure when posting proposals, but please keep them clear and more understandable 🙏

Krishna2323 commented 3 months ago

@hungvu193, I don't see any detailed RCA(same as mine) in the selected proposal and the solution is straight forward so I don't think I have to mention each and every component. I already mentioned we can create a new prop and can be passed similarly to interactive prop. What more details are you looking for?

Krishna2323 commented 3 months ago

@hungvu193, If we address the second issue, why not also tackle the third one? I apologize for the argument 🙏🏻, but I believe it is very unfair to assign someone else’s proposal, which uses a similar approach to mine, especially when it is based on something that wasn't part of the original issue. I humbly request a re-evaluation.

cc: @mountiny

hungvu193 commented 3 months ago

Sorry to let you down, but if you read selected proposal, you can see the different right?

We integrate a map view component to preview the distance map while awaiting the backend to provide the map preview in image format for the IOU report. The ConfirmedRoute component is intended to match the output that the backend will eventually deliver. However, in the staging environment, there are significant discrepancies between the behavior of the ConfirmedRoute component and the backend's expected output:

This's what I'm looking for.

And I don't think it's straight forward enough to put in that short explanation, as I mentioned here.

Anyway, I'll let @mountiny decide.

Krishna2323 commented 3 months ago

@hungvu193, I believe the original issue cares is not showing the center button and my proposal has enough RCA for that, the other proposal tried to point out different issues thats why they included these points in solutions section. I also mentioned about the interactive prop which is used for same purpose (receipt thumbnail). Also, can you pls explain me why aren't we solving the third bug if we are solving the second bug?

Thanks for your responses, I'll wait for @mountiny's views here.

hungvu193 commented 3 months ago

I think my above answer is clear enough. However, that's just my thoughts. Since then I raised an internal discussion here: https://app.slack.com/client/E047TPA624F/C02NK2DQWUX I'll let you know ASAP when we have final decision. Thanks for your patient 🙏

melvin-bot[bot] commented 3 months ago

📣 @hungvu193 🎉 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 3 months ago

📣 @truph01 You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

mountiny commented 3 months ago

I agree @truph01 proposal is a bit more detailed and such we are going to proceed with theirs

Thanks everyone for your proposals, let's move on and focus on other bugs/features

@truph01 please raise the pr

truph01 commented 3 months ago

@hungvu193 PR https://github.com/Expensify/App/pull/43630 is ready for review.

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

https://github.com/Expensify/App/pull/43630

If no regressions arise, payment will be issued on 2024-07-02. :confetti_ball:

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

@hungvu193 requires payment automatic offer (Reviewer)
@truph01 requires payment (Needs manual offer from BZ)

melvin-bot[bot] commented 3 months 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:

[ ] [@hungvu193] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@hungvu193] 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:
[ ] [@hungvu193] 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:
[ ] [@hungvu193] Determine if we should create a regression test for this bug.
[ ] [@hungvu193] 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.
[ ] [@lschurr] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

lschurr commented 3 months ago

Payment summary:

Contributor: @truph01 $250
Contributor+: @hungvu193 $250 (paid in Upwork)

lschurr commented 3 months ago

@truph01 - Are you paid via NewDot or is the automation on the comment incorrect?

truph01 commented 3 months ago

@lschurr The automation looks incorrect, I usually get paid via Upwork.

My Upwork profile link is https://www.upwork.com/freelancers/~01c880ca813fdd85ec

Can you send me the offer?

lschurr commented 3 months ago

Yep! Just sent the offer @truph01

melvin-bot[bot] commented 3 months ago

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

https://github.com/Expensify/App/pull/43630

If no regressions arise, payment will be issued on 2024-07-10. :confetti_ball:

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

@hungvu193 requires payment automatic offer (Reviewer)
@truph01 requires payment (Needs manual offer from BZ)

melvin-bot[bot] commented 3 months 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:

[ ] [@hungvu193] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@hungvu193] 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:
[ ] [@hungvu193] 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:
[ ] [@hungvu193] Determine if we should create a regression test for this bug.
[ ] [@hungvu193] 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.
[ ] [@lschurr] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

truph01 commented 2 months ago

@lschurr Offer accepted TY!

lschurr commented 2 months ago

This is complete :)

Expensify / App