Closed ovan closed 7 years ago
Code looks good!
Some questions:
I kinda understand the rationale for returning 409 vs silently ignoring, but still I started to think more about this: Attempting to block a date that is either already blocked or has a booking on it is silently ignored. I understand that attempt to block already blocked date is silently ignored. But blocking a date that is already booked, isn't that a user error? Shouldn't the API return something saying "No, you can't do that because it's already booked. Cancel the booking first if you want to block it. I don't want to return you status 2xx so that you don't think that everyting went ok. Instead, I'm returning you 4xx, so that you know that what you tried do is not ok". I can't think of any "normal" situation where someone tries to block date that was already booked. But I do can think some error scenarios, e.g. admin is blocking dates from the manage availability view, but at the same time someone just managed to book the date couple seconds before. In that case, I think we need to show an error to the admin. Or what do you think?
Do we want to validate that the duration for each new block is 1 day?
e.g. admin is blocking dates from the manage availability view, but at the same time someone just managed to book the date couple seconds before. In that case, I think we need to show an error to the admin
The problem is the batch nature of the operation. By returning 409 the API would have to reject all blocks. There's no way currently to say "I accepted these 4 but rejected these 2". Adding that would mean a lot of work on the API side. In addition, it would force the client logic to handle the "some succeeded but some failed" case. So I just decided here it's not worth the effort. What the UI should do now is that after submitting blocks it should refetch the bookable. That way it can correctly show which dates are actually blocked and which are booked. I feel it's good enough solution for this case.
I agree that it makes sense to refetch all data when the saving ends, no matter if it succeeds or fails. That keeps the UI in sync with the data better.
@rap1ds In addition to the scenario of someone making a booking after the data is fetched, there is also the possibility of the fetching failing, but the UI letting the user attempt to block/unblock days anyways. We might of course not allow that when the fetching has failed.
API now prevents bookings and blocks overlapping with each other. Attempting to create a booking on a date that is already booked or is blocked is a user error and the API returns 409 conflict. Attempting to block a date that is either already blocked or has a booking on it is silently ignored. The API response for createBlocks return all the blocks that were actually created. If none of the blocks you gave make sense then the API response is 201 with an empty list of created resources in the body.
The logic is that you cannot book a booked/blocked date; that's an error and should be shown to user as a failure. This is different with creating blocks. You simply want to say that given date is not available for booking. If the date was already blocked or booked then nothing changes, the day stays unavailable for booking.