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-10-25] [HOLD for payment 2023-10-16] 🟢 [$2000] Get user's location #25855

Closed
hayata-suenaga opened this issue Aug 24, 2023 · 83 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Design External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@hayata-suenaga
Copy link
Contributor

hayata-suenaga commented Aug 24, 2023

This is a part of the Distance Request project. We're adding a feature with which users can request a reimbursement for gas expenses they incur while traveling by car. Please see the following video to check how this beta feature currently look like. This specific task is related getting the user's location in the flow of creating a distance request.

Video
Screen.Recording.2023-08-24.at.7.30.36.AM.mov

We’ll create a UseCurrentLocationButton which will render a View with styles.flexRow. Inside it will be a pin Expensicon and the text “Use current location”. The UseCurrentLocationButton will be rendered directly below the AddressSearch component.

When the button is pressed we will call getCurrentPosition(success, error, config) which will automatically prompt for the user’s location if needed. In the success callback we will clear any errors, set the text in the WaypointAddress input to “Your location”, access the user’s current location via position.coords.latitude/longitude, save the waypoint in Onyx with address: ‘Your location’, and then navigate back to the “Request money” page.

The getCurrentPosition function is from the new react-native-x-geolocation library

In the error callback we’ll call an action setLocationError() which will set the errorCode on a location object in Onyx. We’re using the errorCode instead of a message because we want to let the component handle displaying the proper error message. We’ll use a new component called LocationErrorMessage to display the error below the button. It will have a similar appearance to the DotIndicatorMessage but it will show one error at a time.

If the error.code is 1 for PERMISSION_DENIED then we will display the message "It looks like you have denied permission to your location. Please [allow location permission in settings](link to settings) and then try again”. We will render this error message using Text components and translation keys for the message before and after the link piece. The text in brackets will use a TextLink so that Linking.openURL will be able to open the settings on native and desktop. On web we will link to a new help.expensify.com page where there will be instructions for how to access location settings on each browser. Most modern browsers do not allow pages to link to settings for security reasons.

To fetch the link we’ll add to react-native-x-geolocation a function called getPermissionDeniedHelpLink(helpLink, settingsLink = 'app-settings:LOCATION_SETTINGS'). For the platform specific implementations we simply return the correct link. Although the function is a bit trivial it is configurable from the App side and handles the platform specific piece.

If I understand correctly, getPermissionDeniedHelpLink is a very simple function that returns helpLink if the code is running on Web and settingsLink if it's running on iOS or Android

If the error code is 2 for POSITION_UNAVAILABLE or 3 for TIMEOUT then we’ll use DotIndicatorMessage and display an error message like “We were unable to find your location, please try again or enter an address manually”.

Screenshot 2023-08-24 at 7 17 00 AM

internal link

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0152e9315a07451f08
  • Upwork Job ID: 1695128490480984064
  • Last Price Increase: 2023-08-25
  • Automatic offers:
    • huzaifa-99 | Contributor | 26330544
@neonbhai
Copy link
Contributor

Hi Contributor here! I am available to work on this feature since the requirements are straight-forward and clear. I would love to help and raise a PR quickly!

@thienlnam
Copy link
Contributor

I've gotten started on this but going to let a contributor finish this up so I can focus on some other issues.

Whoever decides to pick this up can use this if it helps #25868
There are also some pending changes to react-native-x-geolocation that will be merged in this PR Expensify/react-native-x-geolocation#8

@hayata-suenaga hayata-suenaga added Help Wanted Apply this label when an issue is open to proposals by contributors External Added to denote the issue can be worked on by a contributor labels Aug 25, 2023
@melvin-bot melvin-bot bot changed the title Get user's location [$1000] Get user's location Aug 25, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 25, 2023

Job added to Upwork: https://www.upwork.com/jobs/~0152e9315a07451f08

@melvin-bot
Copy link

melvin-bot bot commented Aug 25, 2023

Triggered auto assignment to @mallenexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Aug 25, 2023

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

@hayata-suenaga hayata-suenaga changed the title [$1000] Get user's location [$2000] Get user's location Aug 25, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 25, 2023

Upwork job price has been updated to $2000

@hayata-suenaga
Copy link
Contributor Author

doubling the payout amount

@b4s36t4
Copy link
Contributor

b4s36t4 commented Aug 25, 2023

Would love to work on this.

@Nikhil-Vats
Copy link
Contributor

Hey @hayata-suenaga, I'd like to work on this, do we need to follow the proposal template here? Asking since the PR description is quite detailed.

@hayata-suenaga
Copy link
Contributor Author

@neonbhai @b4s36t4 @Nikhil-Vats

I want an initial PR in 24 hours and finish most of the work over the weekend by Monday. Are you available this weeked to work on this?

@Nikhil-Vats
Copy link
Contributor

Yes @hayata-suenaga

@ishpaul777
Copy link
Contributor

I can work on this @hayata-suenaga

@b4s36t4
Copy link
Contributor

b4s36t4 commented Aug 25, 2023

Yes, will do.

@shubham1206agra
Copy link
Contributor

@hayata-suenaga I would also like to work on this

@b4s36t4
Copy link
Contributor

b4s36t4 commented Aug 25, 2023

But whose PR will get accepted? I think all the contributors would be will be willing to work, since this is a 2000$ PR.

@shubham1206agra
Copy link
Contributor

But whose PR will get accepted? I think all the contributors would be will be willing to work, since this is a 2000$ PR.

And its not basic like FC migration

@neonbhai
Copy link
Contributor

@hayata-suenaga Hi! I already started working on the details after @thienlnam's comment, and have a plan ready! I will be working on this fulltime over the weekend and can get this moving quickly if you assign this to me.

@huzaifa-99
Copy link
Contributor

huzaifa-99 commented Aug 25, 2023

Proposal

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

Not a problem actually, We want to add a new feature for the Distance Request project.

This feature will allow users to request a reimbursement for expenses they incur while traveling by car.

This ticket is scoped to get the user's location in the flow of creating a distance request.

What is the root cause of that problem?

No root cause, just a new feature.

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

Step 1: Create UseCurrentLocationButton

For UI, it will

  1. Render a View (with styles.flexRow)
  2. There will be a pin Expensicon inside with the text “Use current location”.

The UseCurrentLocationButton will be rendered directly below the AddressSearch component in the Distance request.

For functionality it will

  1. On press call getCurrentPosition(success, error, config) which will automatically prompt for the user’s location if needed.

OnSuccess It will

  • clear any errors
  • access the user’s current location via position.coords.latitude/longitude
  • save the waypoint in Onyx with address
  • set the text in the WaypointAddress input to the current location of the user
  • and navigate back to the “Request money” page.

The getCurrentPosition function will come from the react-native-x-geolocation library which we added

OnError -> It will call an action setLocationError() which will set the errorCode on a location object in Onyx.

The errorCode is to be used instead of a message because we want to let the component handle displaying the proper error message.

  • If the error.code is 1 for PERMISSION_DENIED then we will display the message "It looks like you have denied permission to your location. Please [allow location permission in settings](link to settings) and then try again”.
  • If the error code is 2 for POSITION_UNAVAILABLE or 3 for TIMEOUT then we’ll use DotIndicatorMessage and display an error message like “We were unable to find your location, please try again or enter an address manually”.

Step 2: Create LocationErrorMessage

The error message (using errorcode) will be rendered by LocationErrorMessage and will be displayed below the UseCurrentLocationButton.

It will have a similar appearance to the DotIndicatorMessage but it will show one error at a time.

For UI

  1. We will be using Text components (and translation keys for the message).
  2. The text in brackets will use a TextLink so that Linking.openURL can work on native/desktop devices.

For Functionality

  1. On web we will link to a new help.expensify.com page where there will be instructions for how to access location settings on each browser.
  2. On mobile we will have to get the settings link and use that to redirect user.

There is some modifications that we will have to do on this i.e To fetch the link we’ll add to react-native-x-geolocation a function called getPermissionDeniedHelpLink that will return helpLink on web and settingsLink on native

Step 3 - Implement UseCurrentLocationButton in Distance Request Page

Finally we will have to add UseCurrentLocationButton in the distance request page and make sure everything works as expected.

What alternative solutions did you explore? (Optional)

N/A

--

I can work on this over the weekend with an initial PR in 24 hours and finish most of it by coming Monday.

@hayata-suenaga
Copy link
Contributor Author

I love how detailed @huzaifa-99 's proposal is. Gonna assign you

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 25, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

  • when @huzaifa-99 got assigned: 2023-08-25 18:16:07 Z
  • when the PR got merged: 2023-10-17 00:41:14 UTC
  • days elapsed: 36

On to the next one 🚀

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Oct 18, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-10-16] 🟢 [$2000] Get user's location [HOLD for payment 2023-10-25] [HOLD for payment 2023-10-16] 🟢 [$2000] Get user's location Oct 18, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 18, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2023

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

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:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot melvin-bot bot added the Overdue label Oct 27, 2023
@hayata-suenaga
Copy link
Contributor Author

hayata-suenaga commented Oct 27, 2023

waiting for checklist and payments

@melvin-bot melvin-bot bot removed the Overdue label Oct 27, 2023
@mallenexpensify
Copy link
Contributor

mallenexpensify commented Oct 30, 2023

@hayata-suenaga can you help breakdown who is owed what here? I see three contributors assigned and it looks like there are multiple PRs and one was reverted.

@mallenexpensify the first PR was revereted and we're implementing and reviewing the second PR right now

I created an Upwork job, so those who are due money can apply there

@hayata-suenaga
Copy link
Contributor Author

$2,000 for @huzaifa-99

and corresponding C+ payments for @parasharrajat and @narefyev91

@huzaifa-99 did any other C+ reviewed your PRs?

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Oct 31, 2023

Issue reporter: N/A
Contributor: @huzaifa-99 paid $ 2000 via Upwork
Contributor+: @parasharrajat due $2000 via NewDot
Contributor+: @narefyev91 paid via CallStack

@huzaifa-99, can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01488f192a3e781db8

Thanks @hayata-suenaga !

@huzaifa-99
Copy link
Contributor

@huzaifa-99 did any other C+ reviewed your PRs?

No

@huzaifa-99, can you please accept the job and reply here once you have?
$2,000 for @huzaifa-99

I accepted the offer but @hayata-suenaga I am not sure if I am eligible for this since the original PR was reverted and then a follow-up PR was created.

cc: @mallenexpensify

@hayata-suenaga
Copy link
Contributor Author

but this issue is for both the original PR and the follow-up PR right?

did we have another issue and you got paid already?

@huzaifa-99
Copy link
Contributor

but this issue is for both the original PR and the follow-up PR right?

yes

did we have another issue and you got paid already?

i didn't got paid, and we managed the original and followup pr in this issue.

@hayata-suenaga
Copy link
Contributor Author

Then the payment summary @mallenexpensify provided seems correct 👍

You did really good job with your PRs and I'm impressed with your perseverance to finish the task despite of its difficulty 🙇

@huzaifa-99
Copy link
Contributor

TYSM @hayata-suenaga

@mallenexpensify
Copy link
Contributor

Updated the payment comment above.

Contributor: @huzaifa-99 paid $ 2000 via Upwork

@hayata-suenaga since this is new, is there a test we want to add for regressions? I'm leaning towards no but am unsure.

@hayata-suenaga
Copy link
Contributor Author

It might be quite late to ask this question but I've been always wondering this; "is a regression test something that QA always do to each of every RP or deployment?"

@mallenexpensify
Copy link
Contributor

Good question @hayata-suenaga . My understanding is that there are a suite of regression tests that get run for each deployment. Then, there are also tests that they only run once a month, which is often for design-related issues or edge cases.

@parasharrajat
Copy link
Member

We can close this ticket. I will request the payment later.

@parasharrajat
Copy link
Member

Payment requested as per #25855 (comment)

@JmillsExpensify
Copy link

$2,000 payment approved for @parasharrajat based on this comment.

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 Design External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests