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

event_type reserved now explicitly means taking possession #421

Conversation

dirkdk
Copy link
Contributor

@dirkdk dirkdk commented Jan 2, 2020

Explain pull request

The status_change event_type reserved has caused confusion (see #417). More and more providers support reservation as in future hold for a vehicle. It seemed that reserved was actually not meant for this use case, but just for taking possession. The requirement to have a trip_id hints at this. This PR more explicitly states the intent of the event_type reserved to be only when a user takes physical possession of the vehicle, not to reserve for future use.

Is this a breaking change

Although the API definitions strictly remain the same, both API clients and implementations may have to be adjusted.

  • I'm not sure

Impacted Spec

  • provider

Additional context

Agency has trip_start and reserved, this means that Provider reserved is comparable to Agency trip_start

@dirkdk dirkdk requested a review from a team as a code owner January 2, 2020 04:59
@dirkdk dirkdk requested a review from a team January 2, 2020 04:59
@claassistantio
Copy link

claassistantio commented Jan 2, 2020

CLA assistant check
All committers have signed the CLA.

@dirkdk dirkdk changed the title Event_type reserved now explicitly means taking possession Agency: event_type reserved now explicitly means taking possession Jan 5, 2020
@sarob sarob added bug Something isn't working Provider Specific to the Provider API labels Jan 5, 2020
@sarob sarob added this to the Future milestone Jan 8, 2020
@thekaveman thekaveman modified the milestones: Future, 0.4.1 Jan 21, 2020
Copy link
Collaborator

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good clarification of the original intent. A follow-up PR to bring some notion of future-hold reservations is probably warranted for a subsequent release.

@thekaveman thekaveman changed the title Agency: event_type reserved now explicitly means taking possession event_type reserved now explicitly means taking possession Jan 21, 2020
@thekaveman thekaveman added documentation documentation change can be for code and/or markdown pages and removed bug Something isn't working labels Jan 29, 2020
@thekidder
Copy link
Contributor

What happens when a device is reserved but not yet in a customer's possession? Should the vehicle remain in the available state, despite the fact that it is not rentable?

I think it would be helpful to add a new event_type_reason for the unavailable state to address this.

A proposal to clarify this, though it is a breaking change:

  • Rename the existing reserved event_type to on_trip or similar
  • Add a new reserved event_type_reason to the unavailable state.

This accurately captures the distinction here.

@Retzoh
Copy link
Contributor

Retzoh commented Jan 30, 2020

What happens when a device is reserved but not yet in a customer's possession? Should the vehicle remain in the available state, despite the fact that it is not rentable?

I think it would be helpful to add a new event_type_reason for the unavailable state to address this.

A proposal to clarify this, though it is a breaking change:

  • Rename the existing reserved event_type to on_trip or similar
  • Add a new reserved event_type_reason to the unavailable state.

This accurately captures the distinction here.

This would re-conciliate the provider and agency APIs as far as trip and reservation are concerned. We could event add a cancel_reservation to the available state.

(linking to #271)

@marie-x
Copy link
Collaborator

marie-x commented Jan 30, 2020

@Retzoh trip would match Agency. I think adding trip and change the meaning of reserve would be good in the 0.5.0 timeframe.

Copy link
Collaborator

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided on the 1/30 call that this change might in fact break historical data created under the ambiguous understanding/definition of reserved. We will keep the PR around for posterity but will not merge and look for @dirkdk to submit a follow-up with perhaps a note calling out the ambiguity for 0.4.1.

@dirkdk
Copy link
Contributor Author

dirkdk commented Jan 31, 2020

closing this for PR #439

@dirkdk dirkdk closed this Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation documentation change can be for code and/or markdown pages Provider Specific to the Provider API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants