-
Couldn't load subscription status.
- Fork 361
fix offset bug when base_interval_validation.type is not hourly #452
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
Conversation
spec/examples/weekly_rule_spec.rb
Outdated
| end | ||
| end | ||
|
|
||
| context 'when base interval validation type is not hourly' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we describe the scenario instead of the implementation please?
Something like: "with an hour of day that is misaligned to the start time", or similar if I'm understanding the whole issue here. You can run rspec --format=doc on this file and see what makes sense here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated language somewhat. The more I dig into this, the more niche this bug seems.
- schedule with both weekly intervals and hour of day rules to trigger the
base_interval_validation.typemismatch - a
start_timethat is misaligned with thedayrule so that start_time modification due tospans: trueis not defaulted back tostart_time
I can perhaps nest more contexts if that would be preferred?
It's also a bit tricky as to where exactly this spec should exist. The change was made in hour_of_day, so I'm happy to relocate it there if that feels more relevant.
The discovery process was very much outside-in, so apologies for any extra confusion.
| context 'when base interval validation type is not hourly' do | ||
| let(:start_time) { Time.utc(2018, 8, 7, 7, 0, 0) } | ||
| let(:end_time) { Time.utc(2018, 8, 7, 15, 0, 0) } | ||
| let(:biweekly) { IceCube::Rule.weekly(2).day(:saturday).hour_of_day(10) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are both day and hour_of_day needed for this scenario? I'd like to keep this isolated to the single issue at hand if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed on wanting to simplify the scenario. I was unable to reproduce this bug without both validation requirements also being present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it only affects weekly schedules, then I think it belongs here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for digging into this!
| context 'when base interval validation type is not hourly' do | ||
| let(:start_time) { Time.utc(2018, 8, 7, 7, 0, 0) } | ||
| let(:end_time) { Time.utc(2018, 8, 7, 15, 0, 0) } | ||
| let(:biweekly) { IceCube::Rule.weekly(2).day(:saturday).hour_of_day(10) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it only affects weekly schedules, then I think it belongs here.
bug described in #451