fix(UXP-858): add offer_request_id inside Offers list operations #68
Conversation
lapa182
requested review from
igorp1 and
mcdeguzman99
and removed request for
a team
src/booking/Offers/Offers.ts
Outdated
| */ | ||
| public list = (options?: PaginationMeta): Promise<DuffelResponse<Offer[]>> => | ||
| this.request({ method: 'GET', path: this.path, params: options }) | ||
| public list = (offer_request_id: string, options?: ListOffersParams): Promise<DuffelResponse<Offer[]>> => |
There was a problem hiding this comment.
What do you thinking of putting offer_request_id inside the params object instead?
The benefit of the current way is that it makes this argument "required". But it will make Offers.list look inconsistent when compared to other list functions, which only have an options parameter.
There was a problem hiding this comment.
I like the idea of having it separate since we are separating a mandatory argument, which is offer_request_id from the additional options (limit, sort, etc). Even so, it diverges from the others list functions, most of the developers would understand that. Happy to hear other thoughts too! @igorp1 @mcdeguzman99 ?
There was a problem hiding this comment.
I think it makes the library's API a little harder to reason about personally, and it will make it harder in the future for us to automatically generate code snippets in the API reference. But I'm happy for you guys to make the call.
There was a problem hiding this comment.
My preference would be to keep required fields explicit and separate, and to put all of the optional fields in options
There was a problem hiding this comment.
I think it should go inside options to be consistent with the rest of the client library interface, but it should be a required argument.
When we have an id as a separate argument (usually in get functions), it's because it's the ID of the resource that we're referring to - e.g. if you do offers.get(id), you're passing in the offer's ID. In this case we're talking about the offer request ID. This is a different but related object. A similar example can be found in SeatMaps, where you get a seatmap not by its own ID, but by the related offer's ID. Hence the offer ID is passed in as a required parameter.
src/booking/Offers/Offers.ts
Outdated
| */ | ||
| public list = (options?: PaginationMeta): Promise<DuffelResponse<Offer[]>> => | ||
| this.request({ method: 'GET', path: this.path, params: options }) | ||
| public list = (offer_request_id: string, options?: ListOffersParams): Promise<DuffelResponse<Offer[]>> => |
There was a problem hiding this comment.
I think it should go inside options to be consistent with the rest of the client library interface, but it should be a required argument.
When we have an id as a separate argument (usually in get functions), it's because it's the ID of the resource that we're referring to - e.g. if you do offers.get(id), you're passing in the offer's ID. In this case we're talking about the offer request ID. This is a different but related object. A similar example can be found in SeatMaps, where you get a seatmap not by its own ID, but by the related offer's ID. Hence the offer ID is passed in as a required parameter.
| /** | ||
| * Retrieves a generator of all offers. The results may be returned in any order. | ||
| * @param {Object.<ListOffersParams>} params - Pagination options (optional: limit, after, before) | ||
| * @param {string} params.offer_request_id - Duffel's unique identifier for the offer request, returned when it was created |
There was a problem hiding this comment.
| * @param {string} params.offer_request_id - Duffel's unique identifier for the offer request, returned when it was created | |
| * @param {string} offer_request_id - Duffel's unique identifier for the offer request, returned when it was created |
Since it shouldn't actually be nested inside "params"?
There was a problem hiding this comment.
I was reading about it and because it's nested inside params we need to define the previous object since we are deconstructing the object. Makes sense?
There was a problem hiding this comment.
aah ok, I read that as you need to have it as {params: {offer_request_id: ...}}
| * Retrieves a page of offers. The results may be returned in any order. | ||
| * @param {Object} [options] - Pagination options (optional: limit, after, before) | ||
| * @param {Object.<ListOffersParams>} params - Pagination options (optional: limit, after, before) | ||
| * @param {string} params.offer_request_id - Duffel's unique identifier for the offer request, returned when it was created |
There was a problem hiding this comment.
| * @param {string} params.offer_request_id - Duffel's unique identifier for the offer request, returned when it was created | |
| * @param {string} offer_request_id - Duffel's unique identifier for the offer request, returned when it was created |
There was a problem hiding this comment.
LGTM! And just to check... looks like there's enough consensus on this discussion, right?
What was done?
offer_request_idargument since it's required in the API to retrieve the offers listmax_connectionsandsortto OfferRequest types