Monthly rules with more BYDAYs are not always displayed correctly in the first month

RESOLVED FIXED in 4.0.0.1

Status

Calendar
Internal Components
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Decathlon, Assigned: Decathlon)

Tracking

Lightning 3.1
4.0.0.1

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Not sure if it has already been filed a bug, but Stefan Sitter reported this issue in bug 485912 comment 6.
It prevented to use the rule BYDAY=MO,TU,WE,TH,FR,SA,SU for the case "Every day" of a month.

Steps to reproduce:
Import the following calendar that represents the monthly rule "every day" of every month starting from the first of January 2014:

BEGIN:VCALENDAR
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
VERSION:2.0
BEGIN:VEVENT
CREATED:20090529T202927Z
LAST-MODIFIED:20090529T203001Z
DTSTAMP:20090529T203001Z
UID:fbacc989-a182-4def-8c76-15219d89689c
SUMMARY:Monthly Every Day
RRULE:FREQ=MONTHLY;BYDAY=SU,MO,TU,WE,TH,FR,SA
DTSTART;VALUE=DATE:20140101
DTEND;VALUE=DATE:20140102
TRANSP:TRANSPARENT
END:VEVENT
END:VCALENDAR

Actual result:
Lightning displays wrongly the first week of the first month (January) because days 2,3,4,5 are missing.
The other months are displayed correctly.

The same rule starting from September 2014 doesn't cause any issue:

RRULE:FREQ=MONTHLY;BYDAY=SU,MO,TU,WE,TH,FR,SA
DTSTART;VALUE=DATE:20140101
DTEND;VALUE=DATE:20140102

In the following rule, where in BYDAY there is the fourth Monday, Lightning displays only occurrences in the last week of the first month, days from 2 to 26 are missing:

RRULE:FREQ=MONTHLY;BYDAY=SU,4MO,TU,WE,TH,FR,SA
DTSTART;VALUE=DATE:20140101
DTEND;VALUE=DATE:20140102
(Assignee)

Comment 1

3 years ago
Created attachment 8413355 [details] [diff] [review]
31.0a1monthlyFirstMonth-v2.patch

As far as I can see when the code searches for the first occurrence by setting the date "last" to the proper value, it considers only the first element in BYDAY (with the order MO, TU, WE, TH, FR, SA, SU), the other elements are not considered at all because there isn't any loop for the indexes of this.by_data.BYDAY (or impl->by_ptrs[BY_DAY] in libical).

This patch ckecks all the BYDAYs and sets "last" to the first day of the month that has a corrispondence in the BYDAYs.

Not sure how it should treat days with position prefixes that would give days of the month outside the month such as 7MO or -8WE. The patch behaves like the original code, but I'm not certain that
(Assignee)

Comment 2

3 years ago
Sorry, an accidentally click on the touchpad.
> days of the month outside the month such as 7MO or -8WE. The patch behaves
> like the original code, but I'm not certain that...
...it is correct. Anyway I've done some testing with wrong values like those mentioned above and both the original code and the patch seems working fine.
(Assignee)

Comment 3

3 years ago
(In reply to Decathlon from comment #0)
> The same rule starting from September 2014 doesn't cause any issue:
> 
> RRULE:FREQ=MONTHLY;BYDAY=SU,MO,TU,WE,TH,FR,SA
> DTSTART;VALUE=DATE:20140101
> DTEND;VALUE=DATE:20140102
> 

Correction:
the rule starting form September 2014 actually is

RRULE:FREQ=MONTHLY;BYDAY=SU,MO,TU,WE,TH,FR,SA
DTSTART;VALUE=DATE:20140901
DTEND;VALUE=DATE:20140902
(Assignee)

Comment 4

3 years ago
Created attachment 8546862 [details] [diff] [review]
patch libical with test

I added a test to the patch. Not sure if other kind of recurrence rule should be tested, that one in the test summarizes the problem.
The code for ical.js is now necessary to pass the tests.
Attachment #8413355 - Attachment is obsolete: true
Attachment #8546862 - Flags: review?(philipp)
Comment on attachment 8546862 [details] [diff] [review]
patch libical with test

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

I think the tests are sufficient, r=philipp with the following answered:

::: calendar/base/modules/ical.js
@@ +4657,5 @@
> +          parts = this.ruleDayOfWeek(this.by_data.BYDAY[i]);
> +          dayOfMonth = this.last.nthWeekDay(parts[1], parts[0]);
> +          if (dayOfMonth < matchingDay) {
> +            if (dayOfMonth <= 0) {
> +              matchingDay = 0;

What happens when setting this.last.day to zero? Would that normalize to the last day of the previous month?
Attachment #8546862 - Flags: review?(philipp) → review+
(Assignee)

Comment 6

3 years ago
Created attachment 8561717 [details] [diff] [review]
patch libical-ical.js with test - v2

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

> What happens when setting this.last.day to zero? Would that normalize to the
> last day of the previous month?

Trying to verify the case, I tested rules with BYDAYs that go outside the month and I came across the case of the fifth weekday (BYDAY=5xx) that is reported in bug 419490.

What if the month doesn't have the fifth xx weekday? In my opinion the right way is to ignore the occurrences for months that don't have the 5th weekday (like a BYMONTHDAY=31 in February) and this is what the ical.js code does for all the "last" except the first one, that is the one searched with the code related to this patch.
During the iterator initialization, the code uses "this.last" as index in the loop and the automatic normalization makes useless the upper/downer limit of the loop (the code exits from the loop only by means of the internal "break") and hence considers the first xx weekday of the next month as the fifth xx weekday of the previous month.
 
The patch, instead, restarts the research of the 5xx weekday in the next month(s) when it doesn't exist in the current month. In this way bug 419490 is fixed too (the tests passed with BYDAY=5xx and -5xx).

As a consequence of this approach all the BYDAYs with |pos|>=6 should be treated as invalid (Malformed data), for a *monthly* rule, otherwise a new bug arises. I didn't find an explicit note on rfc5545 about that, but it seems reasonable.
Attachment #8546862 - Attachment is obsolete: true
Attachment #8561717 - Flags: review?(philipp)
Comment on attachment 8561717 [details] [diff] [review]
patch libical-ical.js with test - v2

Lets do it, r=philipp
Attachment #8561717 - Flags: review?(philipp) → review+
(Assignee)

Comment 8

3 years ago
Pushed to comm-central
https://hg.mozilla.org/comm-central/rev/52a0b245d6a9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.0
You need to log in before you can comment on or make changes to this bug.