941425 - Yearly rule "Last day of a month" can't be set with the UI and is wrongly displayed in the views.

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[mozillabug]

Attachment: file test.ics and a screenshot

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131112160018

Steps to reproduce:

Thunderburd 24.1.0 + Lightning 2.6.2
Connect to this distant calendar "test.ics" (with wamp or easyphp) :
-----------------------------------------
BEGIN:VCALENDAR
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
VERSION:2.0
X-WR-CALNAME:test
X-WR-TIMEZONE:Europe/Paris
BEGIN:VTIMEZONE
TZID:Europe/Paris
X-LIC-LOCATION:Europe/Paris
BEGIN:DAYLIGHT
TZOFFSETFROM:+0100
TZOFFSETTO:+0200
TZNAME:CEST
DTSTART:19700329T020000
RRULE:FREQ=YEARLY;BYDAY=-1SU;BYMONTH=3
END:DAYLIGHT
BEGIN:STANDARD
TZOFFSETFROM:+0200
TZOFFSETTO:+0100
TZNAME:CET
DTSTART:19701025T030000
RRULE:FREQ=YEARLY;BYDAY=-1SU;BYMONTH=10
END:STANDARD
END:VTIMEZONE
BEGIN:VEVENT
CREATED:20131121T034558Z
UID:20131121034558-7573944@localhost
X-auteur:leiopar(ppra)
X-validateur:jerome(ppra)
SUMMARY:test2
RRULE:FREQ=YEARLY;BYMONTHDAY=-1;BYMONTH=2
DTSTART;TZID=Europe/Paris:20091019T080000
DTEND;TZID=Europe/Paris:20091019T080000
DESCRIPTION:
BEGIN:VALARM
ACTION:DISPLAY
TRIGGER;VALUE=DURATION:-P3D
DESCRIPTION:Description par défaut
END:VALARM
END:VEVENT
END:VCALENDAR


Actual results:

The "test2" event is on 30 January and not on 28 (or 29) February
(the but is the same with another mouth)


Expected results:

The "test2" event must go to 28 (or 29) February

Comment 1

[Decathlon]

I can confirm this issue with both libical and ical.js.
Lightning also can't manage this rule in the UI since the drop-down menus in the Edit recurrence dialog don't allow to set a rule with the last day of a month in a yearly recurrence.

Comment 2

[mozillabug]

"Lightning also can't manage this rule in the UI since the drop-down menus in the Edit recurrence dialog don't allow to set a rule with the last day of a month in a yearly recurrence."
Yes... but another soft allow that, so...

Comment 3

[Decathlon]

Attachment: 28Lastday-libical.patch

This is the part that corrects the position of the occurrences in the views.
I ask for feedback before the complete patch since I used ical.js to find out the issue and then I've applied the changes also to libical (tested on both after building).
At the end it seems the same problem that caused bug 386516.

The patch also fixes the rule:
FREQ=YEARLY;BYMONTHDAY=-1
wrongly displayed one month and one day earlier than the month of the start date.

Comment 4

[Decathlon]

Adding the "last day" in the UI for the yearly rule also implies to add a "every day" option. 
Currently we treat the "every day" case for the monthly rule with the BYMONTHDAY from 1 to 31 and it works fine:

RRULE:FREQ=MONTHLY;INTERVAL=x;BYMONTHDAY=1,2,3,4,5,6,7,8,9,10,11,12,13,14,
 15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31
 
but it seems that Lightning displays wrongly that rule for the YEARLY case, e.g. in a "every day of Febrauary" rule:

RRULE:FREQ=YEARLY;BYMONTH=2;BYMONTHDAY=1,2,3,4,5,6,7,8,9,10,11,12,13,14,
 15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31

Lightning doesn't ignore the last days that exceed the last day of the month and it places them in the next month (in this case three days are extended to March) and, if I don't miss something, this should be a wrong interpretation of the rule.

In any case, I think we should consider to treat the "every day" case with the *BYDAY* parameter instead of BYMONTHDAY because it generates a shorter rule, more suitable for yearly rule where we can set a single month with known length hence using days until 31 would be useless in many cases (differently from the monthly rule where different months included in the rule can have different length so a "1 to 31" is necessary) and, last but not least, because I've seen that the few examples on rfc5545 use a way like that (although it doesn't exist a specific rule):

RRULE:FREQ=MONTHLY;INTERVAL=x;BYDAY=MO,TU,WE,TH,FR,SA,SU

RRULE:FREQ=YEARLY;BYMONTH=y;BYDAY=MO,TU,WE,TH,FR,SA,SU

What's your opinion?

Comment 5

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

Yes, I think BYDAY makes more sense here. I think its also what some other clients use.

Comment 6

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

Comment on attachment 8344160 [details] [diff] [review]
28Lastday-libical.patch

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

The code looks fine in general, I think the best way forward would be to write some tests for ical.js and see if this behaves well in all cases.

::: calendar/base/modules/ical.js
@@ +5351,5 @@
>            for (var monthdaykey in this.by_data.BYMONTHDAY) {
> +            var day_ = this.by_data.BYMONTHDAY[monthdaykey];
> +            var month_ = this.by_data.BYMONTH[monthkey];
> +            if (day_ < 0) {
> +              var daysInMonth = ICAL.Time.daysInMonth(month_, aYear);

Not sure if its worth it, but given this is in a double for loop maybe the result of daysInMonth should be cached for aYear.

Comment 7

[Decathlon]

(In reply to Philipp Kewisch [:Fallen] from comment #5)
> Yes, I think BYDAY makes more sense here. I think its also what some other
> clients use.

Actually I had forgotten the reason why we didn't use the rule BYDAY=MO,TU,WE,TH,FR,SA,SU for the "every day" case. Lightning doesn't display correctly that rule in the first month (see bug 485912 comment 6). Since then the behavior has slightly changed but the problem still arises.

Another issue arises for YEARLY recurrence with more BYDAYs, hence the "every day" case BYDAY=MO,TU,WE,TH,FR,SA,SU, that concerns this bug, would be a particular case affected by the issue.

I've opened two bugs for these issues bug 958974 and bug 958978. Since I think I've found the cause for both (and a fix) we could definitely use the BYDAY=MO,TU,WE,TH,FR,SA,SU for the "every day" case.

Comment 8

[Decathlon]

Attachment: CompletePatch-v1

(In reply to Philipp Kewisch [:Fallen] from comment #6)
> >            for (var monthdaykey in this.by_data.BYMONTHDAY) {
> > +            var day_ = this.by_data.BYMONTHDAY[monthdaykey];
> > +            var month_ = this.by_data.BYMONTH[monthkey];
> > +            if (day_ < 0) {
> > +              var daysInMonth = ICAL.Time.daysInMonth(month_, aYear);
> 
> Not sure if its worth it, but given this is in a double for loop maybe the
> result of daysInMonth should be cached for aYear.

Here you mean in the other piece of code where there is the double for loop.
I moved  var daysInMonth  between the two for loops.



Overall this patch:

- fixes the wrong display in the view of the rule  FREQ=YEARLY;BYMONTH=x;BYMONTHDAY=-1 (the bug in the description);

- adds a menuitem "day" to the dropdown list in the yearly rule (Edit Recurrence dialog) in order to manage the rule mentioned above and handles all the new options that come with it:
  "the last _day_";
  "the first|the second .... _day_";
  "every _day_".
included the description strings.

- fixes the wrong display in the view of the rule  FREQ=YEARLY;BYMONTHDAY=-1 (it was displayed one month earlier) and the related sentence in the description string;

- fixes the recurrence string of the rule RRULE:FREQ=YEARLY;BYMONTH=x because it uses the month of the start date instead of the month in the BYMONTH parameter (the controls in the Recurence dialog are set correctly though);

- fixes the recurrence string and the settings in the dialog's controls for the rule RRULE:FREQ=YEARLY;BYMONTHDAY=x because it takes the day of the start date instead of the day of the month specified in the BYMONTHDAY parameter;  

- changes the way to handle the "every day" case for both monthly and yearly rules as mentioned in the previous comments with the rule BYDAY=MO,TU,WE,TH,FR,SA,SU. The issue of the wrong displayed event in the first month is going to be fixed in bug 958974.

  
Moreover I have a doubt if the file calUtils.js is the right place for the function arrayHasElementsEqIndexes(). Does it need to be more robust (e.g. check on the types of parameters). Do you have also any suggestion for a better name?


(In reply to Philipp Kewisch [:Fallen] from comment #6)
> I think the best way forward would be to
> write some tests for ical.js and see if this behaves well in all cases.

I need assistance here.
For now I'm only able to provide a calendar with a set of events that cover the elements touched by the patch.

Comment 9

[Decathlon]

Attachment: CalendarTest

This is a calendar with a few events with rules that concern the patch.
In the description the events report the rule and the results that would be achieved without the patch.
The events are a bit overlapped so a number in the summary helps to identify the correct element in the view.

Comment 10

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

Comment on attachment 8411849 [details] [diff] [review]
CompletePatch-v1

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

> - adds a menuitem "day" to the dropdown list in the yearly rule (Edit
> Recurrence dialog) in order to manage the rule mentioned above and handles
> all the new options that come with it
Does this work for l10n? I can imagine there might be a locale where the word order is different for "the last wednesday" and "the last day".

I've taken a look at the code here which seems fine aside from a few nits. I haven't had a chance to test it yet though.

r=philipp for the patch, I'll comment on tests in a moment.

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +303,5 @@
>              var day_of_week = Number(getElementValue("monthly-weekday"));
>              if (day_of_week < 0) {
>                  if (ordinal == 0) {
> +                    // Monthly rule "Every day of the month".
> +                    recRule.setComponent("BYDAY", 7, [ 2, 3, 4, 5, 6, 7, 1 ]);

Maybe we can make this array [2,3,4,5,6,7,1] a constant? Its used more than once and it looks magical.

::: calendar/base/src/calUtils.js
@@ +1881,5 @@
> + * @param aArray          the array to check;
> + * @param aArrayLength    an integer that allows to verify
> + *                        the length of the array;
> + */
> +function arrayHasElementsEqIndexes(aArray, aArrayLength) {

Not sure I understand this function:

for every element of the array
  check each element of the array
    to find an element that is equal to the next position

???

Just from your description it seems like:

for (var i = 0; i < aArray.length; i++) {
  if (aArray[i] != (i+1)) {
    return false;
  }
}
return true;

Or maybe this, which I think we could do even without a helper:

aArray.every(function(elem, index) elem == index + 1);

Comment 11

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

Sorry, got carried away. Here is my comment on tests:

First of all, you asked about the ical.js tests. Its fairly easy, just clone, execute a command and open your browser. I've done a small writeup here <https://github.com/mozilla-comm/ical.js/wiki/Running-Tests>, if there is anything you don't understand right away, please let me know so I can update the wiki page. For this kind of test I'd like to see something that specifically covers all branches of just the code you have changed in ical.js.


Next, I think it would be good to create a mozmill recurrence test to cover the Lightning UI integration. Markus has been working on getting those running in bug 980515, so you can take a look at how those work. If you want to run it on the tryserver, you need the patch from bug 680203 (it won't apply fully, but only because I forgot to backout one file. Just ignore that file and all should be good). If you need more help on getting the mozmill tests running, don't hesitate to contact me or Markus.

Comment 12

[Decathlon]

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

> Does this work for l10n? I can imagine there might be a locale where the
> word order is different for "the last wednesday" and "the last day".

We have the same case in the monthly rule where there are two dropdown menus with ordinal items in one of them and the week days in the other with the "Day of the month" item in last position. Probably the same item can't be used in the yearly case because of the presence of another dropdown menu with months so I used a label "day", but the issue you describe would be the same. I'm not aware of a l10n issue in the monthly case.
Do I have to add the l12y keyword or add someone in cc? 
 

> ::: calendar/base/src/calUtils.js
> @@ +1881,5 @@
> > + * @param aArray          the array to check;
> > + * @param aArrayLength    an integer that allows to verify
> > + *                        the length of the array;
> > + */
> > +function arrayHasElementsEqIndexes(aArray, aArrayLength) {
> 
> Not sure I understand this function:
> 

Yes, actually I had to use a different description. Let's say that the function checks for the presence of all the number from 1 to n inside an n-length array but in *every order*, hence arrays such as

  [1, 2, 3, 4, 5, 6, 7 ]      [2, 3, 4, 5, 6, 7, 1 ]      [5, 3, 1, 2, 4, 6, 7 ]
  [SU,MO,TU,WE,TH,FR,SA]      [MO,TU,WE,TH,FR,SA,SU]      [TH,TU,SU,MO,WE,FR,SA]
   
are all good for the rule "ever day" of the month and the function returns true in all the cases. Instead, according to functions like those you proposed, only the first one would be valid.
Usually the order is like the second case (starting from Monday) but I'm not 100% sure about the sequence that might happen. To avoid problems, I took in count every order of the elements and in this way I don't need to verify about the way the array to check is achieved and how and where it comes in the origin (rule.getComponent(), BYDAY, etc.) i.e. if it's for sure always sorted and, in this case, in which way is sorted.
Anyway if there is a simpler way to implement it, it's well accepted ;-)

About the position of the function, I only needed a place to write it once and make it available for two different files, but since its use is limited to these cases, I will put it in the files where it needs and I'll call it from there.

Comment 13

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

(In reply to Decathlon from comment #12)
> (In reply to Philipp Kewisch [:Fallen] from comment #10)
> 
> > Does this work for l10n? I can imagine there might be a locale where the
> > word order is different for "the last wednesday" and "the last day".
> ...
> Do I have to add the l12y keyword or add someone in cc? 
I doubt someone will answer, I guess the only way to figure it out is to implement it and wait for complaints :-}


> Yes, actually I had to use a different description. Let's say that the
> function checks for the presence of all the number from 1 to n inside an
> n-length array but in *every order*, hence arrays such as
> 
>   [1, 2, 3, 4, 5, 6, 7 ]      [2, 3, 4, 5, 6, 7, 1 ]      [5, 3, 1, 2, 4, 6,
> 7 ]
>   [SU,MO,TU,WE,TH,FR,SA]      [MO,TU,WE,TH,FR,SA,SU]     
> [TH,TU,SU,MO,WE,FR,SA]

In that case, this should work:

function checkAllWeekdays(days, len) {
  var mask = days.reduce(function(v, c) v | (1 << c), 1);
  return days.length == len && mask == Math.pow(2, len + 1) - 1;
}

Comment 14

[Decathlon]

(In reply to Philipp Kewisch [:Fallen] from comment #13)
> In that case, this should work:
> 
> function checkAllWeekdays(days, len) {
>   var mask = days.reduce(function(v, c) v | (1 << c), 1);
>   return days.length == len && mask == Math.pow(2, len + 1) - 1;
> }

Wow! Nice.
You should patent it :-))

Comment 15

[Decathlon]

Attachment: CompletePatch+test - v2.patch

Addressed observations in comment 10:
- deleted the function from calUtils.js and added your function in two separated files. I've made it less generic, just for the scope, only 7-lenght array oriented.
- used a constant for the "magical" array.


Tests still are a complete mystery to me. I've found out a file test_recur.js that seems a good place for recurring tests like these about yearly recurrence, so I've added two recurring events for the two cases that the patch changes in libical and ical.js.
Let me know if it's a good solution or it needs something different.

Comment 16

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

Comment on attachment 8422257 [details] [diff] [review]
CompletePatch+test - v2.patch

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

(In reply to Decathlon from comment #15)
> Created attachment 8422257 [details] [diff] [review]
> CompletePatch+test - v2.patch
> 
> Tests still are a complete mystery to me. I've found out a file
> test_recur.js that seems a good place for recurring tests like these about
> yearly recurrence, so I've added two recurring events for the two cases that
> the patch changes in libical and ical.js.
> Let me know if it's a good solution or it needs something different.

Given that ical.js is also used in other projects, unfortunately we need to write an ical.js test for it and push it upstream. Feel free to leave in the test_recur code though, this way we can cover the libical code.

The process for getting the ical.js changes in is as follows. If you don't have experience with git, here is a cheat sheet: https://github.com/sympy/sympy/wiki/Git-hg-rosetta-stone. The biggest difference for the simple case is that you have to git add -u before you commit.

1) Create patch and send pull request on github:
   - I'd suggest putting tests here: https://github.com/mozilla-comm/ical.js/blob/master/test/recur_iterator_test.js
   - As mentioned earlier: https://github.com/mozilla-comm/ical.js/wiki/Running-Tests
   - After changing ical.js' lib/ical/recur_iterator.js, call "make package"
2) Copy ical.js' build/ical.js into Lightning's calendar/base/modules/ical.js and update the hash (use git log to get the hash)
3) Upload comm-central patch here.

If you'd like to import the ical.js changes for this issue and the other one you are working on in one step, please file a new bug to import the changes and make it block the two bugs. 

I know this is a little unwieldy and I'm happy to help if you'd like. Just email me if you need anything.

Comment 17

[Decathlon]

Attachment: Updated patch without ical.js

Philipp, I've updated the patch but I've removed the ical.js part because, as I told you with a mail on June, I can't make working the tests on GitHub (on Windows a lot of problems arose compared to the instructions on the page .../ical.js/wiki/Running-Tests). I will open a new bug for the ical.js part as soon as I can find a way to run the tests.

This patch built correctly on the try server and passed the tests for the libical part so, if you agree, it should be ready for check in.

Comment 18

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

Sounds good, setting checkin-needed. I'm leaving the needinfo to remind me to try setting up tests on windows.

Comment 19

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

Decathlon, I've updated the Running-Tests wiki page with the extra note to run make test-agent-config beforehand to generate the config file. This worked for me on windows using mozilla-build as my shell and the official node.js binary package. Running some of the other make targets I did have path issues.

The other thing that should be working if you can't get the browser tests to run is running tests via node on the command line, I've added instructions to the wiki page as well.

Comment 20

[Richard Marti (:Paenglab)]

https://hg.mozilla.org/comm-central/rev/aa932bf6d9ab

Leaving open because of comment 18.

Comment 21

[Decathlon]

After a long fight (I reinstalled everything), now I get the tests working with the browser so I added a patch with test on GitHub. Don't know if I did everything in the right way there, it seems a bit chaotic, I suspect I shouldn't open the issue #117.
Anyway, is it possible to add the fix to ical.js on comm-central once it has been approved upstream or it has to be added along with the other fixes added lately about other bugs?

Comment 22

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

darktrojan opened a bug to pull the latest ical.js. I got started on that, but unfortunately some changes need to be made for the tests to pass since I landed support for the jCal final and there were some slight API changes. You can land to c-c if you like, the changes will get overridden by latest ical.js once that lands anyway.

Comment 23

[Geoff Lankow (:darktrojan)]

I pulled the changes from ical.js in bug 1117456, so that the unit tests pass.

Comment 24

[Decathlon]

(In reply to Geoff Lankow (:darktrojan) from comment #23)
> I pulled the changes from ical.js in bug 1117456, so that the unit tests
> pass.

So the ical.js' part has landed on comm-central and hence we can close this.