olin-build / ABE

Amorphous Blob of Events
https://abe.olin.build/
GNU Affero General Public License v3.0
7 stars 1 forks source link

Updates usage of rrule to only use rCount or rUntil #190

Closed mpbrucker closed 6 years ago

mpbrucker commented 6 years ago

As per #177, fixes usage of rrule to avoid inconsistencies with the iCal spec. The spec specifies:

                       ; The UNTIL or COUNT rule parts are OPTIONAL,
                       ; but they MUST NOT occur in the same 'recur'.
                       ;

This does somewhat change the behavior of recurrence to prioritize UNTIL over COUNT when both are provided. However, considering that the front-end already only allows one option, and that the spec doesn't provide any guidance as to which one should be used over the other, this seems like a reasonable option.

Another thing to consider is that this will somewhat change the events that are currently returned, since it appears that the database has events that feature both rCount and rUntil, and it seems that with the old behavior rrule was prioritizing COUNT.

mpbrucker commented 6 years ago

After doing a bit more digging, I found that two things I changed should probably actually be different. Firstly, rCount should actually take precedence over rUntil. This is because, in recurrent events that have a count field, there is no until field, which causes this line to assign the query end to rUntil, which then overrides rCount! This results in an event with count 10 that is queried over a long period of time returning way more occurrences than it should.

Secondly, a change I made earlier to adjust rStart based on start also causes issues, because for events with count, if the start of the rrule is shifted forward, it ignores the earlier events - so if you query starting halfway through an event with 10 occurrences, it still returns 10 events.

I've made both of these changes and added more unit tests to cover these cases.