Excludes times when they fall out of range

[maxx-coffee]

Currently there seems to be a bug when you have exceptions that fall out of range of the schedule.

Example:

schedule =
      ~N[2017-09-09 09:00:00]
      |> Cocktail.Schedule.new(duration: 1800)
      |> Cocktail.Schedule.add_recurrence_rule(:secondly,
        interval: 1800,
        hours: [9, 10, 11, 12, 13, 14, 15],
        until: ~N[2017-09-10 23:59:00]
      )
      |> Schedule.add_exception_time(~N[2017-09-09 13:00:00])
      |> Schedule.add_exception_time(~N[2017-09-09 13:00:00])
      |> Schedule.add_exception_time(~N[2017-09-09 13:30:00])
      |> Schedule.add_exception_time(~N[2017-09-09 17:00:00])
      |> Schedule.add_exception_time(~N[2017-09-10 10:00:00])

    times = schedule |> Schedule.occurrences() |> Enum.to_list()

Results in:

[
  %Cocktail.Span{from: ~N[2017-09-09 09:00:00], until: ~N[2017-09-09 09:30:00]},
  %Cocktail.Span{from: ~N[2017-09-09 09:30:00], until: ~N[2017-09-09 10:00:00]},
  %Cocktail.Span{from: ~N[2017-09-09 10:00:00], until: ~N[2017-09-09 10:30:00]},
  %Cocktail.Span{from: ~N[2017-09-09 10:30:00], until: ~N[2017-09-09 11:00:00]},
  %Cocktail.Span{from: ~N[2017-09-09 11:00:00], until: ~N[2017-09-09 11:30:00]},
  %Cocktail.Span{from: ~N[2017-09-09 11:30:00], until: ~N[2017-09-09 12:00:00]},
  %Cocktail.Span{from: ~N[2017-09-09 12:00:00], until: ~N[2017-09-09 12:30:00]},
  %Cocktail.Span{from: ~N[2017-09-09 12:30:00], until: ~N[2017-09-09 13:00:00]},
  %Cocktail.Span{from: ~N[2017-09-09 13:30:00], until: ~N[2017-09-09 14:00:00]},
  %Cocktail.Span{from: ~N[2017-09-09 14:00:00], until: ~N[2017-09-09 14:30:00]},
  %Cocktail.Span{from: ~N[2017-09-09 14:30:00], until: ~N[2017-09-09 15:00:00]},
  %Cocktail.Span{from: ~N[2017-09-09 15:00:00], until: ~N[2017-09-09 15:30:00]},
  %Cocktail.Span{from: ~N[2017-09-09 15:30:00], until: ~N[2017-09-09 16:00:00]},
  %Cocktail.Span{from: ~N[2017-09-10 09:00:00], until: ~N[2017-09-10 09:30:00]},
  %Cocktail.Span{from: ~N[2017-09-10 09:30:00], until: ~N[2017-09-10 10:00:00]},
  %Cocktail.Span{from: ~N[2017-09-10 10:00:00], until: ~N[2017-09-10 10:30:00]},
  %Cocktail.Span{from: ~N[2017-09-10 10:30:00], until: ~N[2017-09-10 11:00:00]},
  %Cocktail.Span{from: ~N[2017-09-10 11:00:00], until: ~N[2017-09-10 11:30:00]},
  %Cocktail.Span{from: ~N[2017-09-10 11:30:00], until: ~N[2017-09-10 12:00:00]},
  %Cocktail.Span{from: ~N[2017-09-10 12:00:00], until: ~N[2017-09-10 12:30:00]},
  %Cocktail.Span{from: ~N[2017-09-10 12:30:00], until: ~N[2017-09-10 13:00:00]},
  %Cocktail.Span{from: ~N[2017-09-10 13:00:00], until: ~N[2017-09-10 13:30:00]},
  %Cocktail.Span{from: ~N[2017-09-10 13:30:00], until: ~N[2017-09-10 14:00:00]},
  %Cocktail.Span{from: ~N[2017-09-10 14:00:00], until: ~N[2017-09-10 14:30:00]},
  %Cocktail.Span{from: ~N[2017-09-10 14:30:00], until: ~N[2017-09-10 15:00:00]},
  %Cocktail.Span{from: ~N[2017-09-10 15:00:00], until: ~N[2017-09-10 15:30:00]},
  %Cocktail.Span{from: ~N[2017-09-10 15:30:00], until: ~N[2017-09-10 16:00:00]}
]

The expectation is %Cocktail.Span{from: ~N[2017-09-10 10:00:00], until: ~N[2017-09-10 10:30:00]} and %Cocktail.Span{from: ~N[2017-09-09 13:00:00], until: ~N[2017-09-09 13:30:00]} should not show up.

[luizmiranda7]

There are no occurrences for 5 pm, so there's no reason to add an exception time for it. Also, we can remove the duplicated 1 pm exception time with no impact on the test assertions.

Suggested change

	|> Schedule.add_exception_time(~N[2017-09-09 13:00:00])
	|> Schedule.add_exception_time(~N[2017-09-09 13:00:00])
	|> Schedule.add_exception_time(~N[2017-09-09 13:30:00])
	|> Schedule.add_exception_time(~N[2017-09-09 17:00:00])
	|> Schedule.add_exception_time(~N[2017-09-10 10:00:00])
	|> Schedule.add_exception_time(~N[2017-09-09 13:00:00])
	|> Schedule.add_exception_time(~N[2017-09-09 13:30:00])
	|> Schedule.add_exception_time(~N[2017-09-10 10:00:00])

[maxx-coffee]

There are no occurrences for 5 pm, so there's no reason to add an exception time for it

I think there is a valid reason to test for this. I was playing the the idea of having an availability calendar that allowed users to book certain appointments.

Users have booking hours that they can be booked. We will state that users can book hours m-f 09:00 - 12:00 and 13:00 - 17:00. The user also has a vacation on a specific thursday and friday.

You could handle this by iterating over each time segment to see if a date falls within the time off and exclude those. Or you could add exceptions for the times in between those hours even if they are not in the schedule.

However, if you feel this does not coincide with the way the library should function I'll be happy to take the suggestion. Playing devils advocate a bit, I think there is a valid reason to have exceptions even if there are not time segments that fulfill those exceptions.

Also, we can remove the duplicated 1 pm exception time with no impact on the test assertions.

Good point. This test was written in true TDD style. If you run this test currently the double 13:00:00 will break the current implementation. 13:00:00 will still show up in the schedule even though there is an exception.

[luizmiranda7]

Got it, as I said, those are minor points, and now that you explained the reasons for it, I'm totally good with it!

[luizmiranda7]

LGTM, only a few minor comments.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e497f58) 96.57% compared to head (fed4619) 96.59%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #241      +/-   ##
==========================================
+ Coverage   96.57%   96.59%   +0.02%     
==========================================
  Files          21       21              
  Lines         496      499       +3     
==========================================
+ Hits          479      482       +3     
  Misses         17       17              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.