Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.27k stars 2.7k forks source link

[$250] Investigate Google Maps API improvements for NewDot distance requests #45432

Closed rafecolton closed 2 weeks ago

rafecolton commented 1 month ago

Our Google rep informed us of some recent improvements to Google's API, so this issue is to:

  1. Investigate if we could benefit from any performance improvements or cost savings
  2. Implement any of those changes that can be made on the front-end
  3. Work with me or another Expensify engineer to make any changes that need to be made on the back-end, such as updating API key scopes.

Here are the details about the changes that may or may not be relevant:

  • You can update your API Keys related to Distance and Directions to Routes API. The Routes API provided improved performance for calculating directions, distance, and travel time. Routing enhancements also include Toll Calculations (perfect tool for helping reimburse drives who travel through tolls), Stopover Waypoints for safety, and even 2-wheel vehicle routes.
  • The Places API has also been upgraded and some of those changes can have an impact, you can find about the New Places APIs here.
  • All you would need to do is update the API keys that use the Places, Distance, and Directions APIs. The engineers can now fieldmask returns with the new Places API, that is a change vs getting everything returned today with the old Places.
  • Google has combined the pricing for Autocomplete and Address Validation, this will reduce the cost of running both APIs in tandem. Address Validation is also available!

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0168dceb5baaaa8452
  • Upwork Job ID: 1815446319044970434
  • Last Price Increase: 2024-07-29
melvin-bot[bot] commented 1 month ago

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

rafecolton commented 1 month ago

Here's a rough breakdown of how much we use each of the Maps APIs:

Please use that info when evaluating effort vs cost savings for making a change.

melvin-bot[bot] commented 1 month ago

@rafecolton, @rojiphil Whoops! This issue is 2 days overdue. Let's get this updated quick!

rafecolton commented 1 month ago

@rojiphil what is needed to move this forward? @mallenexpensify would you mind taking a look as well in case I missed something?

mallenexpensify commented 1 month ago

@rojiphil can you review the details in the OP and provide feedback and recommendations? We might want to create individual issues for this (I have no idea, it's an option though)

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~0168dceb5baaaa8452

melvin-bot[bot] commented 1 month ago

Current assignee @rojiphil is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 month 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.

mallenexpensify commented 1 month ago

@rojiphil 👀 on the above plz

rojiphil commented 1 month ago

can you review the details in the OP and provide feedback and recommendations? We might want to create individual issues for this (I have no idea, it's an option though)

Sorry for the delayed response here. We use react-native-google-places-autocomplete library to access google Maps APIs. I will have to take a closer look at this while considering the recent improvements in Google Maps API. Will do so in the next few days and revert.

rafecolton commented 1 month ago

Thank you! Once we get a sense of scope for this change, we can discuss if $250 is still an appropriate price or if it should be increased.

lschurr commented 1 month ago

Any update on this one @rojiphil?

rojiphil commented 4 weeks ago

Looking into this today

melvin-bot[bot] commented 4 weeks ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 4 weeks ago

@rafecolton @rojiphil @lschurr this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

rojiphil commented 3 weeks ago

From the FE side, we use Places API > Autocomplete via react-native-google-places-autocomplete library. And I am still investigating this. But I expect to share my findings tomorrow.

Meanwhile, I am not sure who is using the other APIs as mentioned in the breakup here Is the BE internally making use of these APIs? If so, what FE action would trigger these API usages?

rafecolton commented 3 weeks ago

Interesting, yeah I see that in the FE using the Proxy_GooglePlaces api command in src/components/AddressSearch/index.tsx, and it looks like that react native lib might use the place details too.

Digging a bit more into the web code, it looks like we are using these features on the BE or old app front-end:

So it looks like the places api is the only one you could possibly improve on the FE in NewDot

rojiphil commented 3 weeks ago

Digging a bit more into the web code, it looks like we are using these features on the BE or old app front-end:

  • find a place - smartscan
  • geocoding - creating GPS-based expenses, a few other things
  • directions, static maps, roads - generating printable Google map receipts *dynamic maps - rendering maps in js in the old app

Ah! Thanks. That clarifies on where the APIs are used. But that leads me to the following query.

Currently, we use Mapbox in NewDot for showing the maps and use Google’s Places API > Autocomplete for autocomplete functionality. This makes me curious to know as to why we have to depend on two different service providers for the maps functionality. This can lead into additional requirements (i.e. display google logo as we are not using google maps) as mentioned in the following screenshot referenced from here.

Wouldn’t it be neat to use a single service provider for all maps related requirements?
For example, if we are going with Mapbox, we can replace Google’s Places API > Autocomplete with Address Autofill or Search Box And if we are going with Google Maps API, we can replace Mapbox with Google Maps using react-native-maps. For autocomplete though, we need to upgrade from Places API to Places API (New)

What are your thoughts on the above?

Screenshot 2024-07-31 at 10 05 31 PM

rafecolton commented 3 weeks ago

I'm not sure why we chose Mapbox instead of Google - maybe @neil-marcellini knows?

I don't think it's super important to consolidate everything right now. It is a much bigger change than we're investigating here and I'm not aware of any specific problem it solves.

rojiphil commented 3 weeks ago

Yeah, I do agree that it’s a much bigger change but it may be worth it as popularity of google maps is much more than mapbox (react-native-maps has 10x times weekly downloads than map box). Anyway, if we want to continue to use Mapbox then we need to display Google logo as mentioned here.

And specific to migration from Places API to Places API (New), we can make the following changes:

  1. Configure Google Cloud to enable Places API (New) to the API key (preferably a new one) as mentioned here
  2. Use HTTP POST instead of HTTP GET for Autocomplete (New) API uses as mentioned here.
  3. Handle the format changes in response structure for Autocomplete (New) API as mentioned here
  4. Use the required parameter fieldmask for Place Details (New) API as mentioned here. This way we can control the billing by including what we need in field mask
  5. Use session tokens to group autocomplete and place details which will help in reduced billing. This is referenced here.
  6. Handle the format changes related to Place object for in response structure of Place Details (New) API as referenced here

Also note that the react-native-google-places-autocompletelibrary include support for nearbysearch and geocoding but I am not sure if this needs to be in scope as this is not yet used by NewDot.

rafecolton commented 3 weeks ago

Great, thank you for the continued investigation! Based on your findings, and keeping the scope limited to the Places migration as used in NewDot, can you please advise:

  1. What do you think is a fair price for this task?
  2. Is this something you would like to implement? If not, can you please help a volunteer to put up a proposal?

Thanks!

rojiphil commented 3 weeks ago

Great, thank you for the continued investigation! Based on your findings, and keeping the scope limited to the Places migration as used in NewDot, can you please advise:

  1. What do you think is a fair price for this task?
  2. Is this something you would like to implement?

Thanks. And yeah, I can implement this. We would need 2 PRs - one for the changes in react-native-google-places-autocomplete library and another for the FE changes. And we can aim to wind this up in two weeks.

Just that I would need your support in a) ensuring that new Places API are enabled in API key and b) ensuring that the BE proxies the new places api and returns back the results to FE.

Regarding the fair price for this task, I think $3000 seems reasonable for the investigation, design, and implementation. What do you think?

rafecolton commented 3 weeks ago

We discussed internally and agreed on $1000 for the implementation. To avoid confusion, I'm going to create a separate issue for that so you get paid the $250 here for your investigation. On the implementation, are you interested in taking either the C or C+ role? Happy to give you first dibs due to your work in this issue.

rojiphil commented 3 weeks ago

On the implementation, are you interested in taking either the C or C+ role? Happy to give you first dibs due to your work in this issue.

@rafecolton I would be happy to take up the C+ role for the implementation. Thanks.

Oh! On second thought, I think it will be faster if I am the contributor here since I have the maximum context for this issue. Let me be the contributor. Thanks.

rafecolton commented 3 weeks ago

Ok sounds good! I'll create the new issue and get you assigned ❤️

@lschurr can you please ensure @rojiphil gets paid $250 on this issue for the investigative work?

lschurr commented 2 weeks ago

@rojiphil - Offer sent here.

rojiphil commented 2 weeks ago

@rojiphil - Offer sent here.

@lschurr Accepted the offer. Thanks.

lschurr commented 2 weeks ago

All paid! I think we can close this one for the other issue, correct?

rafecolton commented 2 weeks ago

All paid! I think we can close this one for the other issue, correct?

Correct, thanks!

mkhutornyi commented 1 week ago

I'm not sure why we chose Mapbox instead of Google

Maybe @hayata-suenaga has the best context.

rafecolton commented 1 week ago

I don't think it really matters, it is not something we are looking to change at this time.