Decathlon Assignee
	Description • 8 years ago

I think that this issue had been reported or discussed somewhere but I can't find any reference. 

Steps to reproduce:

- set the preference calendar.icaljs to false and import the following event where the recurrence rule is set only with the "MONTHLY" tag (it can't be created with Lightning's UI), so that the recurrence occurs on the day of the start date that is the 31st:

BEGIN:VCALENDAR
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
VERSION:2.0
BEGIN:VEVENT
CREATED:20150120T201548Z
DTSTAMP:20150630T062502Z
UID:xx6ff04e-2558-4d9b-bbb6-e14b342a97d1
SUMMARY:Monthly recurrence on 31st
RRULE:FREQ=MONTHLY
DTSTART:20160531T080000Z
DTEND:20160531T090000Z
END:VEVENT
END:VCALENDAR

--> Lightning provides the following occurrences:

    31/05/2016, 30/06/2016, 31/07/2016, 31/08/2016, 30/09/2016, 31/10/2016, ... 28/2/2017 ...

    (the last day of every month) but occurrences on months with 30 or 28 days should not exist.


After the recent fix on ical.js about Bug 1103187, the same event can be tested with icaljs enabled and the occurrences on 30th or 28th are correctly missing.

	Decathlon Assignee
	Comment 1 • 8 years ago

(In reply to Decathlon from comment #0)
> where the recurrence rule is set only with the "MONTHLY" tag (it can't be
> created with Lightning's UI)

This is wrong because the event can be created in the event dialog by setting the start date on the 31st of a month and selecting the monthly recurrence in the Repeat dropdown menu.
Instead it can't be created with the UI if we want to add an interval of months for the recurrence greater than one.


The patch discards the dates with days greater than the last day of the month.
I've also added a test for bug 1103187 that was completely missing.

	Philipp Kewisch [:Fallen] [:📆][:🧩]
	Comment 2 • 8 years ago

Comment on attachment 8743441 [details] [diff] [review]
patch-v1

Review of attachment 8743441 [details] [diff] [review]:
-----------------------------------------------------------------

I assume this works in ical.js?

::: calendar/test/unit/test_recur.js
@@ +368,5 @@
>                 false);
>  
> +    // Bug 1103187 - Monthly recurrence with only MONTHLY tag in the rule. Recurrence day taken
> +    // from the start date. Check four occurrences.
> +    check_recur(createEventFromIcalString("BEGIN:VCALENDAR\nBEGIN:VEVENT\n" +

Might make sense to add the same testcases upstream to the ical.js tests too?

	Decathlon Assignee
	Comment 3 • 8 years ago

(In reply to Philipp Kewisch [:Fallen] from comment #2)

> I assume this works in ical.js?

Yes, it has been fixed with bug 1103187. Libical didn't have entirely that bug but just this particular case (the portion of code is the same).

> Might make sense to add the same testcases upstream to the ical.js tests too?

Actually these tests had already been added by Geoff Lankow with PR https://github.com/mozilla-comm/ical.js/pull/115 for icaljs. I found this bug while tried to add the same tests in comm-central.


Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/57f4a5bbbb88

	[:MakeMyDay]
	Comment 4 • 8 years ago

This seems to be a candidate for an uplift to esr45, right?

	Philipp Kewisch [:Fallen] [:📆][:🧩]
	Comment 5 • 8 years ago

Although it is nice this one is fixed, I'd rather not uplift unless this is absolutely critical. There haven't been massive user complaints and IIUC this kind of rule is not creatable with the Lightning UI.

	Decathlon Assignee
	Comment 6 • 8 years ago

Unfortunately, as written in comment #1, those kind of events can be created with UI and, unfortunately again, is the simplest way to create them but I agree, it would need more testing by someone else before uplift (after a while the patch writer tends to test always in the same way and the same things).

	Philipp Kewisch [:Fallen] [:📆][:🧩]
	Comment 7 • 8 years ago

Ah ok, missed comment 1. But given it hasn't been complained about much before, I think we should let this bake a little before we uplift.