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 2024-07-10][$250] 'You location' button is displayed on distance request thumbnail #43330

Closed
1 of 6 tasks
m-natarajan opened this issue Jun 8, 2024 · 35 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Jun 8, 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.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:

  1. Navigate to a workspace chat
  2. Make a distance request
  3. 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
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6506112_1717844094622.Screen_Recording_2024-06-08_at_1.52.06_in_the_afternoon.mp4

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 OwnerCurrent Issue Owner: @lschurr
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 8, 2024
Copy link

melvin-bot bot commented Jun 8, 2024

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

@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
Copy link
Author

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

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jun 8, 2024

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.

<View style={[styles.pAbsolute, styles.p5, styles.t0, styles.r0, {zIndex: 1}]}>
<PressableWithoutFeedback
accessibilityRole={CONST.ROLE.BUTTON}
onPress={centerMap}
accessibilityLabel={translate('common.center')}
>
<View style={styles.primaryMediumIcon}>
<Icon
width={variables.iconSizeNormal}
height={variables.iconSizeNormal}
src={Expensicons.Crosshair}
fill={theme.icon}
/>
</View>
</PressableWithoutFeedback>
</View>
</View>

<View style={[styles.pAbsolute, styles.p5, styles.t0, styles.r0, {zIndex: 1}]}>
<PressableWithoutFeedback
accessibilityRole={CONST.ROLE.BUTTON}
onPress={centerMap}
accessibilityLabel={translate('common.center')}
>
<View style={styles.primaryMediumIcon}>
<Icon
width={variables.iconSizeNormal}
height={variables.iconSizeNormal}
src={Expensicons.Crosshair}
fill={theme.icon}
/>
</View>
</PressableWithoutFeedback>
</View>
</View>

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.

<Marker
key="Current-position"
longitude={currentPosition?.longitude ?? 0}
latitude={currentPosition?.latitude ?? 0}
>
<View style={styles.currentPositionDot} />
</Marker>

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.

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

if (!shouldPanMapToCurrentPosition()) {
setCurrentPositionToInitialState();
return;

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)

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

  2. We can disable the press event on the button and style the cursor according to the prop.

@lschurr lschurr added the External Added to denote the issue can be worked on by a contributor label Jun 10, 2024
@melvin-bot melvin-bot bot changed the title 'You location' button is displayed on distance request thumbnail [$250] 'You location' button is displayed on distance request thumbnail Jun 10, 2024
Copy link

melvin-bot bot commented Jun 10, 2024

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

@melvin-bot melvin-bot bot added Overdue Help Wanted Apply this label when an issue is open to proposals by contributors labels Jun 10, 2024
Copy link

melvin-bot bot commented Jun 10, 2024

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

@truph01
Copy link
Contributor

truph01 commented Jun 10, 2024

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?

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

  • In this logic:
    if (showMapAsImage) {
    return (
    <View style={[styles.w100, styles.h100]}>
    <ConfirmedRoute

    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.

  2. Then, we just display this component:

    <View style={[styles.pAbsolute, styles.p5, styles.t0, styles.r0, {zIndex: 1}]}>

    if shouldDisplayUserLocation is true.

  3. Then, just display this component:


    if shouldDisplayUserLocation is true.

  4. Then, an additional condition && shouldDisplayUserLocation to this condition:

    if (utils.areSameCoordinate([coordinate[0], coordinate[1]], [currentPosition?.longitude ?? 0, currentPosition?.latitude ?? 0])) {

  5. 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:

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

Proposal updated

  • Added solutions for extra bugs mentioned here.

@hungvu193
Copy link
Contributor

Reviewing shortly

@hungvu193
Copy link
Contributor

hungvu193 commented Jun 11, 2024

3. Then, just display this component:

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

truph01 commented Jun 11, 2024

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

<Marker
key="Current-position"
longitude={currentPosition?.longitude ?? 0}
latitude={currentPosition?.latitude ?? 0}
>
<View style={styles.currentPositionDot} />
</Marker>

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

@hungvu193
Copy link
Contributor

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

Copy link

melvin-bot bot commented Jun 11, 2024

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

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jun 11, 2024

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

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

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

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

hungvu193 commented Jun 11, 2024

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

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

@mountiny
Copy link
Contributor

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

truph01 commented Jun 13, 2024

@hungvu193 PR #43630 is ready for review.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jun 25, 2024
@melvin-bot melvin-bot bot changed the title [$250] 'You location' button is displayed on distance request thumbnail [HOLD for payment 2024-07-02] [$250] 'You location' button is displayed on distance request thumbnail Jun 25, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jun 25, 2024
Copy link

melvin-bot bot commented Jun 25, 2024

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

Copy link

melvin-bot bot commented Jun 25, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.1-19 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 2024-07-02. 🎊

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

Copy link

melvin-bot bot commented Jun 25, 2024

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:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 2, 2024
@lschurr
Copy link
Contributor

lschurr commented Jul 2, 2024

Payment summary:

@lschurr
Copy link
Contributor

lschurr commented Jul 2, 2024

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

@truph01
Copy link
Contributor

truph01 commented Jul 3, 2024

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

lschurr commented Jul 3, 2024

Yep! Just sent the offer @truph01

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jul 3, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-07-02] [$250] 'You location' button is displayed on distance request thumbnail [HOLD for payment 2024-07-10] [HOLD for payment 2024-07-02] [$250] 'You location' button is displayed on distance request thumbnail Jul 3, 2024
Copy link

melvin-bot bot commented Jul 3, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.3-7 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 2024-07-10. 🎊

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

Copy link

melvin-bot bot commented Jul 3, 2024

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

truph01 commented Jul 9, 2024

@lschurr Offer accepted TY!

@lschurr lschurr changed the title [HOLD for payment 2024-07-10] [HOLD for payment 2024-07-02] [$250] 'You location' button is displayed on distance request thumbnail [HOLD for payment 2024-07-10][$250] 'You location' button is displayed on distance request thumbnail Jul 9, 2024
@lschurr
Copy link
Contributor

lschurr commented Jul 10, 2024

This is complete :)

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 Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
No open projects
Archived in project
Development

No branches or pull requests

6 participants