RRuleSet not working with multiple RRule

michaeltoohig commented 5 years ago

Reporting an issue

Thank you for taking an interest in rrule! Please include the following in your report:

[y] Verify that you've looked through existing issues for duplicates before creating a new one
[y] Code sample reproducing the issue. Be sure to include all input values you are using such as the exact RRule string and dates.
[y] Expected output
[y] Actual output
[y] The version of rrule you are using
[y] Your operating system
[y] Your local timezone (run $ date from the command line of the machine showing the bug)

Code

I have been trying to use rruleset but only the first rrule is calculated when using .all

var r1 = "DTSTART:20190228T141000\nRRULE:FREQ=DAILY;BYDAY=TH;BYHOUR=12,13,14"
var r2 = "DTSTART:20190227T105000\nRRULE:FREQ=DAILY;BYDAY=SA;BYHOUR=20,22,23"

var set = new rrule.RRuleSet()
set.rrule(new rrule.rrulestr(r1))
set.rrule(new rrule.rrulestr(r2))

var dates = set.all(function (date, i) {
  if (i === 10) {
    return false
  }
  return true
})

console.log(dates)

Expected Output

A list of all rrule events included in both rrule strings r1 and r2.

Actual Output

Only the r1 string given to the rruleset is calculated. Switching the order of the r1 and r2 are given to rruleset will calculate the output of r2.

Version

2.6.0 is installed via npm

OS

I am using WSL (debian stretch).

Timezone

running the date returns

Tue Feb 26 15:12:29 DST 2019

but my timezone is +11

davidgoli commented 5 years ago

I think this is a legitimate bug, but fixing it will probably take something of a rewrite.

The problem starts when you call RRuleSet.all. Since you've passed an iterator function to stop iterations at 10, you've exposed this bug. What RRuleSet.all is actually doing is iterating through the first rrule, which is unlimited, and reaching your quota of 10 with only the first rrule. If you didn't pass an iterator, but instead used UNTIL or COUNT properties to limit the rrule, you'd see results from both rules in your result set, since all() iterates through all of the first rrule, then all of the second, etc., then finally sorts all the resulting dates together once they're built. In order to fix this bug, we'd have to find the next date of all of the rrules, then pass the nearest one to the iterator.

michaeltoohig commented 5 years ago

I added UNTIL to each of the rrule strings and instead searched the rruleset using between which does work as I wanted.

I can live with that. I don't really need the all search; between now and a few months ahead from now is fine for what I'm doing.

var r1 = "DTSTART:20190228T141000\nRRULE:FREQ=DAILY;BYDAY=TH;BYHOUR=12;UNTIL=20190528T141000"
var r2 = "DTSTART:20190227T105000\nRRULE:FREQ=DAILY;BYDAY=SA;BYHOUR=12;UNTIL=20190528T141000"

var set = new rrule.RRuleSet()
set.rrule(new rrule.rrulestr(r1))
set.rrule(new rrule.rrulestr(r2))

var dates = set.between(new Date(Date.UTC(2019, 2, 4)), new Date(Date.UTC(2019, 5, 3)))

console.log(dates)

Edit

I also wanted to thank you for your quick response and helpful description of what is going on and caused the error.

Jaimeloeuf commented 4 years ago

I think this is a legitimate bug, but fixing it will probably take something of a rewrite.

The problem starts when you call RRuleSet.all. Since you've passed an iterator function to stop iterations at 10, you've exposed this bug. What RRuleSet.all is actually doing is iterating through the first rrule, which is unlimited, and reaching your quota of 10 with only the first rrule. If you didn't pass an iterator, but instead used UNTIL or COUNT properties to limit the rrule, you'd see results from both rules in your result set, since all() iterates through all of the first rrule, then all of the second, etc., then finally sorts all the resulting dates together once they're built. In order to fix this bug, we'd have to find the next date of all of the rrules, then pass the nearest one to the iterator.

Hi @davidgoli thanks for explaining what is going on, would like to ask if there is any status update on this? This seems to be pretty important, even in the cases where the first rrule is not infinite but have many many occurrences.

This seems to be the expected behaviour when using rruleSet.all() most of the time, unless perhaps the documentation mentions that rruleSet.all is not guaranteed to be ordered?

Lastly, would this affect dates / occurrences added via rrulset.rdate() ? I assume it will also be affected? Thanks!

jkbrzt / rrule