-
Notifications
You must be signed in to change notification settings - Fork 361
Prevent illogical rule combinations #420
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
Changes from 5 commits
7f879fe
c6ef94c
70d2e59
4ba3297
d698e84
7f7d5a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,15 +4,6 @@ class ValidatedRule < Rule | |
|
|
||
| include Validations::ScheduleLock | ||
|
|
||
| include Validations::HourOfDay | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 to splitting these out so we can only have them where they make sense |
||
| include Validations::MinuteOfHour | ||
| include Validations::SecondOfMinute | ||
| include Validations::DayOfMonth | ||
| include Validations::DayOfWeek | ||
| include Validations::Day | ||
| include Validations::MonthOfYear | ||
| include Validations::DayOfYear | ||
|
|
||
| include Validations::Count | ||
| include Validations::Until | ||
|
|
||
|
|
@@ -51,6 +42,14 @@ def other_interval_validations | |
| Array(@validations[base_interval_validation.type]) | ||
| end | ||
|
|
||
| def other_fixed_value_validations | ||
| @validations.values.flatten.select { |v| | ||
|
||
| interval_type = (v.type == :wday ? :day : v.type) | ||
| v.class < Validations::FixedValue && | ||
| interval_type == base_interval_validation.type | ||
| } | ||
| end | ||
|
|
||
| # Compute the next time after (or including) the specified time in respect | ||
| # to the given start time | ||
| def next_time(time, start_time, closing_time) | ||
|
|
@@ -185,6 +184,29 @@ def validation_names | |
| VALIDATION_ORDER & @validations.keys | ||
| end | ||
|
|
||
| def verify_alignment(value, freq, rule_part, options={}) | ||
| @validations[:interval] or return | ||
| interval_validation = @validations[:interval].first | ||
| interval_validation.type == freq or return | ||
| fixed_validations = other_fixed_value_validations | ||
| (last_validation = fixed_validations.min_by(&:value)) or return | ||
|
|
||
| alignment = (value - last_validation.value) % interval_validation.interval | ||
| return if alignment.zero? | ||
|
|
||
| validation_values = fixed_validations.map(&:value).join(', ') | ||
| if rule_part == :interval | ||
| message = "interval(#{value}) " \ | ||
| "must be a multiple of " \ | ||
| "intervals in #{last_validation.key}(#{validation_values})" | ||
| else | ||
| message = "intervals in #{last_validation.key}(#{validation_values}, #{value}) " \ | ||
| "must be multiples of " \ | ||
| "interval(#{interval_validation.interval})" | ||
| end | ||
| yield ArgumentError.new(message) | ||
| end | ||
|
|
||
| end | ||
|
|
||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,11 @@ module Validations::DailyInterval | |
|
|
||
| # Add a new interval validation | ||
| def interval(interval) | ||
| @interval = normalized_interval(interval) | ||
| interval = normalized_interval(interval) | ||
| verify_alignment(interval, :wday, :interval) { |error| raise error } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we yield this error up instead of just
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is so that it gives a more relevant trace, since it appears from the called |
||
| verify_alignment(interval, :day, :interval) { |error| raise error } | ||
|
|
||
| @interval = interval | ||
| replace_validations_for(:interval, [Validation.new(@interval)]) | ||
| clobber_base_validations(:wday, :day) | ||
| self | ||
|
|
||
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.
Maybe worth moving this interval-checking into a util that we can use in both places
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.
Yes, I'm not 100% happy with some of the duplication & super-calling either. I'll see what I can do.