duffelhq / duffel-api-javascript

JavaScript client library for the Duffel API
https://duffel.com/docs
MIT License
36 stars 12 forks source link

Question: What is the reason for different `origin`/`destination` types on `OfferSlice`/`OrderSlice` `Segment`s? #891

Closed Tom-Carpendale closed 8 months ago

Tom-Carpendale commented 9 months ago

Hi,

I understand the need for a different type for the segments on OfferSlices and OrderSlices, due to some fields being unavailable before placing an order, or others being irrelevant after the order is placed, but I'm interested to know more about why OfferSliceSegment destinations/origins are Airports, but on the OrderSlice segments they are Places, implying they could be a City instead.

Looking at the docs it actually seems the destination/origin type is intended to be a Place, even on the offer. What does it mean for a segment to arrive at or depart from a city?

Finally, many of the fields on City are marked as nullable or even undefined, though the docs specify them as required and not nullable (e.g. latitude, longitude, timezone)

Thanks in advance 👍

andrejak commented 9 months ago

Hi, thank you for reporting these.

Destinations on offers should be airports, not places. I believe that should be fixed now:

I think cities are only required to have 4 fields: https://duffel.com/docs/api/v1/cities The rest might be where it can actually be a place instead.

Does that help?

Tom-Carpendale commented 9 months ago

Hi,

Thanks for the extra clarification! Am I right in saying that in that case the lines below should read destination: Airport and origin: Airport instead? https://github.com/duffelhq/duffel-api-javascript/blob/217561de1baffecf73afe51c1a7838938bb14e62/src/booking/Orders/OrdersTypes.ts#L237 https://github.com/duffelhq/duffel-api-javascript/blob/217561de1baffecf73afe51c1a7838938bb14e62/src/booking/Orders/OrdersTypes.ts#L273

This would make them match up with the types on the Offer Slice Segments: https://github.com/duffelhq/duffel-api-javascript/blob/217561de1baffecf73afe51c1a7838938bb14e62/src/booking/Offers/OfferTypes.ts#L464

Less relevant to my use case, but should Slice origin's and destination's also be only Airports? i.e. in these places: https://github.com/duffelhq/duffel-api-javascript/blob/217561de1baffecf73afe51c1a7838938bb14e62/src/booking/Offers/OfferTypes.ts#L368 https://github.com/duffelhq/duffel-api-javascript/blob/217561de1baffecf73afe51c1a7838938bb14e62/src/booking/Orders/OrdersTypes.ts#L303

With regards to the cities, that all makes sense. Should the City type reflect that is does not have these fields? https://github.com/duffelhq/duffel-api-javascript/blob/217561de1baffecf73afe51c1a7838938bb14e62/src/types/shared.ts#L37-L54 https://github.com/duffelhq/duffel-api-javascript/blob/217561de1baffecf73afe51c1a7838938bb14e62/src/types/shared.ts#L12

Thanks again for spending some time looking into this! 🙏

andrejak commented 8 months ago

Sorry for the delayed response! I think you're right about the origin & destinations on order segments and the city type, I've made a PR here: https://github.com/duffelhq/duffel-api-javascript/pull/896 I'm less sure about the slice origin & destinations - for now I'll keep those as matching the docs but will check with the team internally.

Tom-Carpendale commented 8 months ago

Thank you, that looks great 👍

andrejak commented 8 months ago

That PR is merged so I'll close this for now but feel free to open (or another issue) if something comes up :)