Open rafecolton opened 2 months ago
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mkhutornyi (External
)
Triggered auto assignment to @Christinadobrzyn (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.
Not sure if the upwork job was created, going to try re-adding the label
Job added to Upwork: https://www.upwork.com/jobs/~015599eafb1cb0d217
Current assignee @mkhutornyi is eligible for the External assigner, not assigning anyone new.
Per discussion in the linked issue, I'm assigning @rojiphil as the Contributor. @rojiphil, please provide a detailed proposal when you are able for @mkhutornyi to review. Thanks!
š£ @mkhutornyi š An offer has been automatically sent to your Upwork account for the Reviewer role š Thanks for contributing to the Expensify app!
š£ @rojiphil š An offer has been automatically sent to your Upwork account for the Contributor role š Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review š§āš» Keep in mind: Code of Conduct | Contributing š
Thanks @rafecolton for assigning me here. I am looking into this and expect to revert in next couple of days.
Any update on when you will be delivering the first proposal @rojiphil?
Any update on when you will be delivering the first proposal @rojiphil?
Almost there. I will share the update today.
@rafecolton @mkhutornyi A draft PR with the proposed implementation details is available in PR description here. Please have a look. We can take this further based on your feedback.
Thanks for the initial writeup. @mkhutornyi please take a look at the proposal and let me know your thoughts.
In order to keep the discussion all in one place, @rojiphil will you please transfer the proposal here? (Feel free to edit your comment above to include the proposal details.)
I have a few initial questions and comments:
https://maps.googleapis.com/maps/api
, passing along the query parameters from the front-end after injecting/replacing the api key. This PR implements the scope limited to the places migration as used in NewDot.
There is no root cause as this is an improvement.
Dependencies:
react-native-google-places-autocomplete
library which is implemented in this PRPlaces API (New)
in Google Cloud Autocomplete (New)
and Place Details(New)
The implementation details for the relevant steps as mentioned here are as follows:
Let us introduce a isNewPlacesAPI
property in GooglePlacesAutocomplete
component here to introduce support for Places (New)
api. By default, we can set this to false
here so that the current implementation of Places
api is used by default. This way, we do not break existing systems using the old Places
API. Further, to make use of the Places API (New)
, the FE can send isNewPlacesAPI
here.
For Autocomplete (New)
request, we can send a HTTP POST request here and also set the new url for POST i.e. ${url}/v1/places:autocomplete
. Further, we can send the request body
with the supported parameters here.
In addition, we also need to update the API for Place Details (New)
with url ${url}/v1/places/${rowData.place_id}
here
Format changes include changing the request parameters to languageCode
, includedRegionCodes
and locationBias
here. For the response though, we need to filter the results and handle the format changes. This is done here and here
Let us introduce a fields
property in GooglePlacesAutocomplete
component here to specify the desired response fields. By default, we can set this to *
here so that all the fields are sent back. In our case, we set fields
to make use of Location Only (Place Details) SKU thereby reducing our API cost. The specific value of fields
is set here.
To use sessions, we can create a sessionToken
state here and use uuid
library to generate v4 uuids for the session. This session token will be used in every Autocomplete (New)
API here. And the session token will be reset to a new uuid here after using it for the last time in the Place Details (New)
API here
Since the response
has changed Place Details (New)
API, there will not be any status
. Hence, we can depend on id
here and pass the details
to the onPress
handler. The FE saveLocationDetails is modified to support the format changes in the Place Details (New)
API response.
- Are you able to test the current API by making a real API request through our web proxy? If not, how do you plan to test? (i.e. Will you need support from an internal engineer?)
We will need the support of an internal engineer to make the BE changes.
With the current changes, the BE returns 666
code response. So, to test until we are BE ready, I have injected some test data in the code which will be removed once the BE is ready. As soon as the BE is ready, the FE should work seamlessly though.
Can you please add some detail around what specific changes will need to be made to the BE?
- The current implementation in the web API essentially proxies the request to
https://maps.googleapis.com/maps/api
, passing along the query parameters from the front-end after injecting/replacing the api key.- Please detail any changes that will be made to the URL, http method, etc. Please ensure this design will remain backwards-compatible. (We either need to add a new proxy endpoint or update the existing one to differentiate between the requests and support both types.)
The specific changes are as follows:
Current implementation proxies the request to https://maps.googleapis.com/maps/api
. The new implementation should proxy the request to https://places.googleapis.com
for both Autocomplete API (New)
and Place Details (New)
. And, as usual, we need to pass along the query parameters from the FE after injecting/replacing the api key.
I proposed to add new proxy endpoints. That way, our design would be backward-compatible.
For Autocomplete API (New): HTTP Method: POST Query URL begins with /v1/places:autocomplete
For Place Details API (New):
HTTP Method: GET
Query URL begins with /v1/places/${rowData.place_id} where rowData.place_id
represents the placeId of the place for which details are to be fetched.
- Please also detail what additional scopes need to be added to the API key, or if a new key is required. If it's possible to add scopes to the existing key rather than generate a new one, I think that would be preferred, as it will simplify the BE code a bit.
The steps required to enable Places API (New)
to the API key is mentioned here. I think we can update the existing API key itself if it is going to simplify the BE.
reviewing... will provide feedback asap
Hi @mkhutornyi just checking in on this, can you please provide an update on the review? thank you!
I've gone through all discussions in https://github.com/Expensify/App/issues/45432 and https://github.com/Expensify/App/issues/46771 and @rojiphil's draft PRs. The proposal looks great. I think we can go ahead with the implemenation. Looks like https://github.com/FaridSafi/react-native-google-places-autocomplete/pull/943 is waiting for maintainer's review & approval. And need help from engineer to work on backend implementation. App PR can be put on hold (draft) meanwhile. ššš
Triggered auto assignment to @thienlnam, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Awesome thanks for the review @mkhutornyi! @thienlnam can you check this out when you have a moment?
Looks like Rafe is going to be the CME on this issue, so going to un-assign myself
The proposal looks great. I think we can go ahead with the implemenation.
Awesome. Thanks @mkhutornyi for your review.
@rafecolton When we can expect the API key updation and the handling of APIs in BE? Once that is done, I can retest the draft PRs with the changes in BE, remove the test code in library, and move the draft PRs to review so that the maintainer of library can review/merge the updates.
I'm going OOO for a couple weeks starting tomorrow, so it won't be until I get back.
We'll move this to weekly until Rafe is back. @thienlnam do you think there's anything to do while Rafe is gone?
Looks like it's pending some BE changes, so it will just have to wait till he gets back or another internal engineer picks it up
Awesome! Sounds good! thank you for taking a peek!
@rafecolton @rojiphil @Christinadobrzyn @mkhutornyi 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!
Adding an internal label to this since there are some BE components. We'll pick this back up when Rafe is back!
This is on hold for a bit longer while Rafe is away!
@rafecolton is there a TL;DR on why we need to do this?
@rafecolton @rojiphil @Christinadobrzyn @mkhutornyi this issue is now 4 weeks old, please consider:
Thanks!
is there a TL;DR on why we need to do this?
@trjExpensify tl;dr The new new APIs return better data faster for a lower cost. More on what was shared by our Google reps here
just a heads up - I'm going to be ooo 9/4 - 9/10. I don't think we'll need another BZ teammate on this issue, so I'll just keep myself, but if you do, please reach out in Slack for someone to jump in!
@rafecolton Gentle bump on taking this forward. Also, can we please move this issue back to daily
?
is there a TL;DR on why we need to do this?
@trjExpensify tl;dr The new new APIs return better data faster for a lower cost. More on what was shared by our Google reps here
Gotcha, it's pretty light on the upside for Places API there in their message in the respect of something we actually want to use.
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.
Are we saving a lot with this new Places API then?
Are we saving a lot with this new Places API then?
I think the savings are significant. Here is a summary of and advantages for making the changes.
Places API (Old) Reference 1000 Autocomplete Requests: $2.83 1000 Place Details Requests: $17.00
Places API (New) Reference 1000 Autocomplete Requests: $2.83 1000 Place Details Requests: $5.00
Advantages:
Location
Field Mask in Places API (New). The existing Place Details Request sends all details but we do not need all those details. With the introduction of field mask in Places API (New), we can fetch only what we want and, hence, the savings.session
based implementation which clubs AutoComplete
requests made in a session as a single AutoComplete
request from the pricing perspective. So,
1000 autocomplete
requests without session ā> $2.83
1000 autocomplete
requests made in 200 sessions (I.e. 5 autocomplete requests per session) will be considered as 200 autocomplete
requests ā> 5x (i.e. 5000 autocomplete requests) such sessions will incur charge of $2.83Not huge in terms of prioritization @trjExpensify but in terms of $$$ it will pay for itself š
@rafecolton Gentle bump on taking this forward. Also, can we please move this issue back to daily?
Thanks for your patience, I'm catching up from OOO this week, should be able to do that Monday
Thanks for your patience. Getting back to the BE component of this now.
I enabled the new Places API in GCP, working on supporting it through our web API now
@rojiphil did some testing locally. Left a small comment on https://github.com/FaridSafi/react-native-google-places-autocomplete/pull/943 and some changes are needed on the App PR. With those small changes, I got the BE working! So I'm putting up that PR now.
Please proceed with getting https://github.com/FaridSafi/react-native-google-places-autocomplete/pull/943 ready for review and https://github.com/Expensify/App/pull/47085 after that š
@rojiphil @mkhutornyi Are you able to test on Expensify Staging? If not, are you able to get your own API key so you can test e2e locally?
Oh I forgot to mention - I made the existing proxy command backwards-compatible, so we won't need a new command/url
The BE changes were merged and deployed to staging today, so they should be in prod on Monday. We are discussing on the App PR whether or not it will be possible for C/C+ to test on staging.
@rafecolton, @rojiphil, @Christinadobrzyn, @mkhutornyi Eep! 4 days overdue now. Issues have feelings too...
Not overdue
yes, not overdue Melvin.
Saw you pushed some changes @rojiphil and commented on the PRs. I've been swamped today, so I'll try to get to this tomorrow. Making myself the issue owner now since this is waiting on my review.
Just a heads up that I'm going to be travelling for the rest of the week - back on Monday. I'm not going to assign a new BZ teammate since this is being handled at the PR stage.
No problem, thanks for the heads up. @rojiphil thanks for your patience, I've been swamped the last couple days. Hope to get back to this ASAP.
Looking at this now
Coming from https://github.com/Expensify/App/issues/45432, please implement the new Places API as described in this comment.
We'll follow the usual process of starting with a detailed proposal, but I will assign @rojiphil directly as the Contributor here since he has the most context.
I will be the CME and will also help with getting the API key updated/installed.
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @rafecolton