Make occurrences in chronological order even if BYMONTHDAY or BYYEARDAY is not in order #666
Conversation
…AY is not in order The recurrence iterator has a problem where it makes occurrences in the same order as given in BYMONTHDAY or BYYEARDAY, which doesn't make sense - it should always return them in chronological order. This patch fixes it by pre-ordering the days that can be used when the month or year changes. A day that is valid for one month/year may be different or not valid for the next month/year, so the days need to be recomputed each time.
Pull Request Test Coverage Report for Build 8730708550Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
| throw new Error("Malformed values in BYDAY part"); | ||
| } | ||
| if (this.rule.freq == "MONTHLY") { | ||
| if (this.has_by_data("BYDAY")) { |
There was a problem hiding this comment.
Github's UI has failed to figure out that all I've done to this block is indent it. I'm pretty sure the block below (else if (this.has_by_data("BYMONTHDAY")) should also only be used if the rule is monthly.
| // Get a sorted list of days in the starting month that match the rule. | ||
| let normalized = this.normalizeByMonthDayRules( | ||
| this.last.year, | ||
| this.last.month, | ||
| this.rule.parts.BYMONTHDAY | ||
| ).filter(d => d >= this.last.day); | ||
|
|
||
| if (normalized.length) { | ||
| // There's at least one valid day, use it. | ||
| this.last.day = normalized[0]; | ||
| this.by_data.BYMONTHDAY = normalized; | ||
| } else { | ||
| // There's no occurrence in this month, find the next valid month. | ||
| // The longest possible sequence of skipped months is February-April-June, | ||
| // so we might need to call next_month up to three times. | ||
| if (!this.next_month() && !this.next_month() && !this.next_month()) { | ||
| throw new Error("No possible occurrences"); | ||
| } | ||
| } |
There was a problem hiding this comment.
I've thrown out an earlier hack and just used the valid days in the starting month if there is at least one of them.
| rule = parseInt(rules[ruleIdx], 10); | ||
| if (isNaN(rule)) { | ||
| throw new Error('Invalid BYMONTHDAY value'); | ||
| } |
There was a problem hiding this comment.
I found that if the rule is a string, weird things happen.
| if (this.has_by_data("BYMONTHDAY")) { | ||
| this.by_data.BYMONTHDAY = this.normalizeByMonthDayRules( | ||
| this.last.year, | ||
| this.last.month, | ||
| this.rule.parts.BYMONTHDAY | ||
| ); | ||
| } |
There was a problem hiding this comment.
This is the important bit (and the same code just below) – go back to the rule as specified and recalculate the valid days for the new month. This deals with negative rules and puts them all in order.
| let daysInYear = Time.isLeapYear(aYear) ? 366 : 365; | ||
| this.days.sort((a, b) => { | ||
| if (a < 0) a += daysInYear + 1; | ||
| if (b < 0) b += daysInYear + 1; | ||
| return a - b; | ||
| }); |
There was a problem hiding this comment.
And this does the same for BYYEARDAY rules.
The recurrence iterator has a problem where it makes occurrences in the same order as given in BYMONTHDAY or BYYEARDAY, which doesn't make sense - it should always return them in chronological order.
This patch fixes it by pre-ordering the days that can be used when the month or year changes. A day that is valid for one month/year may be different or not valid for the next month/year, so the days need to be recomputed each time.