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

Fix for Discrepancy in CostEstimate model for current android SDK (v1.0.3) (as reported on the lyft developer community forum) #7

Merged
merged 7 commits into from
Oct 20, 2017

Conversation

awahnteh
Copy link

This PR covers a fix for an issue reported early October on the Lyft developer community forum. Here is the link to the issue:
https://devcommunity.lyft.co/t/discrepancy-in-costestimate-model-for-current-android-sdk-v1-0-3/202

The commit that actually has this fix is bcde284.

In addition to the aforementioned fix, I have filled a few more gaps that were in the SDK, for example:

  1. The SDK in general (and in particular, the MockLyftPublicApi) is now aware of 3 additional Lyft Products which I think were added as products since the last time this SDK was updated in February (see commits 7c61fee and bbe009e.):

    • LYFT_PREMIER,
    • LYFT_LUX,
    • LYFT_LUX_SUV
  2. Added several more model types to the SDK for objects received and returned by the API; some of which are polymorphic (See EtaLocation which extends LyftLocation and LyftDriver which extends LyftUser classes as examples). Other examples of models added include (but are not limited to) to (see commit: 395d14d ):

    • MonetaryAmount,
    • RideHistory,
    • RideRequest,
    • RideRequestResponse
  3. Added LyftUserApi and and LyftUserApiRx, which is distinct from the LyftPublicApi and LyftPublicApiRx interface as in it exposes methods that require an access_token from a user -- whereas the LyftPublicApi exposes methods that do not require a user access_token (see commit: 395d14d.). In the same context, I have also modified ApiConfig and LyftApiFactory and abstracted them in such a way that a user can request for a LyftUserApi retrofit client or a LyftPublicApi retrofit client seamlessly. In fact the underlying mechanics that differentiates between a User Api and Public Api retrofit client is the fact that LyftApiFactory will create an OkHttpClient with a RequestInterceptor that passes either the client token via apiConfig#getClientToken() (as is the case with a LyftPublicApi) or the LyftApiFactory will create an OkHttpClient with a RequestInterceptor that passes either the user access token via: apiConfig#getUserAccessToken() . For more details see commit: 395d14d:

    • LyftApiFactory.java: line 27 (for Public Api)
    • LyftApiFactory.java: line 37 (for Public Api)
    • LyftApiFactory.java: line 72 (for Public Api)
    • LyftApiFactory.java: line 78-85 (for User Api)
    • ApiConfig.java: line 7 (for User Api)
    • ApiConfig.java: line 27-37 (for User Api)
    • LyftApiFactory.java: line 43-64 (for User Api)
    • LyftApiFactory.java: line 78-85 (for User Api)
  4. Exposed overloads to the getEtas method in the LyftPublicApi interface. Effectively exposing a way for users of the SDK to make calls to the getEtas end point without the need to pass parameters that are deemed optional by that endpoint. (see commit: 25fd37e.)

Will Love to get your thoughts and feedback.

Awah T-

….CostEstimate` object, including adding JavaDoc comments to fields of the CostEstimate model
…mate object model as well as added descriptions for LYFT_PREMIER, LYFT_LUX and LYFT_LUX_SUV (which were previously not in the model).
…` classes which are both mechanisms to make calls to the Lyft Api wherein a user access token is required.

Also included javadoc annotated classes of both `LyftUserApi` and `LyftUserApiRx` which expose methods for the following rest api endpoints:
`@POST("/v1/rides")`
`@POST("/v1/rides/{ride_id}/cancel")` --> 2 overloads of the cancelRide method to handle optional parameter use cases
`@GET("/v1/eta")` --> 3 overloads of the getEtas method to handle optional parameter use cases
`@GET("/v1/rides")`  --> 4 overloads of the getUserRideHistory method to handle optional parameter usecases
@acityinohio
Copy link
Contributor

This is fantastic @awahnteh! We'll re-run the CLA check shortly, thanks so much for your contribution.

@acityinohio acityinohio reopened this Oct 20, 2017
@acityinohio acityinohio merged commit c063c1e into lyft:master Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants