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

Availability improvements #901

Merged
merged 5 commits into from
Aug 17, 2018
Merged

Availability improvements #901

merged 5 commits into from
Aug 17, 2018

Conversation

lyyder
Copy link
Contributor

@lyyder lyyder commented Aug 16, 2018

Fixes two problems with availability:

  • Makes a second time slots request if the number of bookable day exceeds 90 which is the number of time slots that can be fetched with one request
  • Drops date time information form time slots request

In case max bookable day count exceeds 90 days, fetch a second batch of
time slots.
Copy link
Contributor

@kpuputti kpuputti left a comment

Choose a reason for hiding this comment

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

Check comment. I guess another option would've been to request timeslots as needed, e.g. when the user changes the calendar month. But I guess fetching all timeslots is ok as well.

dispatch(timeSlotsRequest(secondParams)).then(secondBatch => {
const combined = timeSlots.concat(secondBatch);
dispatch(fetchTimeSlotsSuccess(combined));
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a return before the dispatch or a .catch on the promise to handle error in the request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The .catch is left out so that in case of an error the .catch of the outer promise handles it. This way there error action is fired in case either of the time slot requests fail. A succeed action is dispatched when both requests succeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would a return help with handling a failing request here?

Copy link
Contributor

Choose a reason for hiding this comment

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

The failing promise never goes to the outer .catch without the return.

Copy link
Contributor Author

@lyyder lyyder Aug 17, 2018

Choose a reason for hiding this comment

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

Well so it seems. I was sure I had this tested. Well spotted 👍, I'll add the return.

@lyyder
Copy link
Contributor Author

lyyder commented Aug 17, 2018

Added the missing return statement. Requesting time slots on demand might introduce latency in showing the time slots so decided to load the time slots in loadData.

Return the nested promise so that errors from it will be caucht in the
catch block of the outer promise.
@lyyder lyyder merged commit 324baa3 into master Aug 17, 2018
@lyyder lyyder deleted the max-time-slots branch August 17, 2018 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants