1103187 - Monthly recurring events appear on first day of month

Status: RESOLVED FIXED

(Calendar :: ICAL.js Integration, defect)

Description

[Geoff Lankow (:darktrojan)]

I have a recurring event on the 25th of every month. It now appears on 2013-08-25 (first occurrence), and the 1st of every month after that.

Comment 1

[Geoff Lankow (:darktrojan)]

Just reproduced it on a non-Google calendar, so that's not the cause. Can anyone confirm this happens and isn't just me?

Comment 2

[[:MakeMyDay]]

Confirming. In 3.3.1, this only happens with icaljs enabled, libical works as expected. The latter is also the case on current daily. With icaljs it cannot be tested there at the moment due to bug 1081534, but I assume it will show up there as well.

Comment 3

[Geoff Lankow (:darktrojan)]

It looks like some of my events have

> RRULE:FREQ=MONTHLY

and some have

> RRULE:FREQ=MONTHLY;BYMONTHDAY=30

and ical.js isn't handling the former type correctly.

Comment 4

[Decathlon]

Attachment: patch - v1

This bug is indirectly caused by the fix for bug 899326 .
With the fix, the function increment_month() sets this.last.day to 1 in order to handle the automatic normalization of the "last" date.
In a case like this where only the tag "MONTHLY" is part of the rule, the code at the end chooses the day as the minimum between 1 and the last day of the month.

This patch should fix but the code should be added upstream on GitHub with a test that at the moment I'm not able to make working.
Meanwhile, if someone wants to test the patch or wants to make a patch completed with test, he/she is welcome.

Comment 5

[Geoff Lankow (:darktrojan)]

That change seems to work as expected. I've created a test.
https://github.com/darktrojan/ical.js/compare/1103187-recur

Comment 6

[Decathlon]

Well done!
Could you also add an event with the same rule but created on the 31st of a month with 31 days? It should repeat every month with 31 days but should be absent for the others months (and not present the last day of the month or shifted few days later).
This works with ical.js with the patch, but it doesn't work with libical, I will open another bug for this specific case.

Comment 7

[Geoff Lankow (:darktrojan)]

No idea what the review process for ical.js is, but I created https://github.com/mozilla-comm/ical.js/pull/115 anyway.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I look at it then merge it :-) Thanks for the PR!

Comment 9

[MooreCandy16]

LEAVE ME LONE I ALREADY WENT TO THE POLICE THEY ARE GONNA GET YOU ASS

Comment 12

[Jorg K (CEST = GMT+2)]

Perhaps we can fix this before it causes more duplicates.

Comment 13

[Jorg K (CEST = GMT+2)]

Please also look at bug 1179783. There is a weekly event that goes bust.

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Since there is a patch, I think its just a matter of landing it on c-c. Note I am working on a whole new recurrence iterator which will hopefully land on github master in the next 3-4 weeks, we should retest to make sure it still works afterwards.

Comment 15

[Decathlon]

(In reply to Philipp Kewisch [:Fallen] from comment #14)
> Since there is a patch, I think its just a matter of landing it on c-c. Note
> I am working on a whole new recurrence iterator which will hopefully land on
> github master in the next 3-4 weeks, we should retest to make sure it still
> works afterwards.

What if we push this patch in ical.js on comm-central in the meanwhile?
We have only to remember to back it out or to change ical.js in the patch for bug 1115667 before pushing that patch when its moment will come.

Comment 17

[Decathlon]

Deleting needinfo since bug 1115667 has landed.

Comment 18

[aleth [:aleth]]

Should this be resolved fixed now?

Comment 19

[Decathlon]

(In reply to aleth [:aleth] from comment #18)
> Should this be resolved fixed now?

I've realized that there isn't a specific test for this issue. I wanted to add the same tests that Geoff Lankow added for this bug on GitHub but I bump into bug 1265554 (a particular case of this one but for libical).
I will add a test for this bug in the patch for bug 1265554.

I think this can be resolved fixed.