Wrong first occurrence for Monthly rule with BYMONTHDAY in particular cases #232

Decathlon1578 · 2016-04-26T16:37:54Z

This PR covers issues which occurs with monthly rules with BYMONTHDAY.
The first occurrence is not always calculated correctly when the BYMONTHDAY, as absolute value, is greater than the last day of the month where the event starts or the last day of the month where the occurrence should occur in case of INTERVAL in the rule.
Mainly the issues are described in Lightning's bugs:
https://bugzilla.mozilla.org/show_bug.cgi?id=1266797
https://bugzilla.mozilla.org/show_bug.cgi?id=386516#c11

coveralls · 2016-04-26T17:07:39Z

Coverage increased (+0.01%) to 96.925% when pulling f7cd86f on Decathlon1578:monthlyBymonthday into 9b80e67 on mozilla-comm:master.

kewisch

I apologize for the long delay! I have some comments that I hope we didn't already discuss in a Mozilla bug. Let me know what you think!

kewisch · 2017-01-02T13:57:46Z

lib/ical/recur_iterator.js

      this.last.month = this.setup_defaults("BYMONTH", "MONTHLY", this.dtstart.month);
+      var daysInMonth = ICAL.Time.daysInMonth(this.last.month, this.last.year);
+      while (Math.abs(day) > daysInMonth) {


What happens if someone wrongly specifies BYMONTHDAY=32 ? Would this be caught somewhere else or would the loop keep spinning? Maybe you could add a test for this too.

kewisch · 2017-01-02T13:58:19Z

lib/ical/recur_iterator.js

+        daysInMonth = ICAL.Time.daysInMonth(this.last.month, this.last.year);
+      }
+      if (day < 0) {
+        day = daysInMonth + day + 1;


day += daysInMonth + 1

kewisch · 2017-01-02T13:59:53Z

lib/ical/recur_iterator.js

        }

        if (day > daysInMonth) {
          this.last.day = 1;
          data_valid = this.is_day_in_byday(this.last);
-        } else {
+        } else if (day > 0) {
          this.last.day = day;
        }


Is there a possibility for an else case? If so there should be some sort of fallback.

mschroeder · 2017-06-23T18:46:59Z

@Decathlon1578 I would like to continue working on your pull request, can you enable edits from maintainers for it? Then I can directly push an update to your branch and pull request.

Decathlon1578 · 2017-07-02T21:35:06Z

@mschroeder
Hi Martin, sorry for the late answer.
As I wrote on the related Lightning's bug on Bugzilla, the problem that Philipp pointed out in the review, i.e. BYMONTHDAYs>=32, is actually caught by the rule parser, though there aren't explicit tests so I've already added a pair in the next patch. The real problem are malformed rules that put together BYMONTH with BYMONTHDAY. It doesn't depend on this patch but it might be corrected by adding few code lines.
For example, a monthly rule with BYMONTH=2 and BYMONTHDAY=31, which is correct for the parser, is catastrophic because ical.js/libical code hangs and makes Thunderbird/Lightning impossible to use and to restart.

A simply fix might be done simply by adding a counter in the patch's |while| loop. Even if it's a bit rough, it would throw an error and would keep Lightning functioning . I have to add some tests for these kind of rules and I'm going to upload a new patch.

Decathlon1578 · 2017-07-30T18:17:01Z

After a long fight with GIT (now I'm in the long list of the git heaters) I thought that everything was fine, I'm going to squash the commits relatively to the patch after the review, but now there's a problem with the "grunt package" command.
Please, could someone explain me what went wrong? I get no error on my local branch apart from the replacement of the "end of line" characters.

mschroeder · 2017-07-30T18:54:13Z

Most of the time this happens if you have not installed the same versions of npm packages as Travis CI. To ensure this, delete the node_modules directory and run npm install again, then grunt package to generate new builds and the source map.

… cases

Decathlon1578 · 2017-07-31T17:15:02Z

OK, I'm a bit tired so I surrender.
Running "npm install" doesn't change the issue about Travis CI. I've tried more than once. I've also updated npm (now v. 5.3.0) but the problem persists.
The only thing that puzzles me is that after "npm install" the following message appears:

npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.1.2 (node_modules\fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.1.2: wanted {"os":"darwin","arch":"any
"} (current: {"os":"win32","arch":"ia32"})

I deleted the file package-lock.json (438KB) before the "grunt package" command in order to avoid to commit it because it doesn't exist upstream.

I don't know what I have to do to make it working. Given that the patch works, and passes all the tests included the new ones, if someone else want to complete the work, I've activate the flag "Allow edits from maintainers". Well, maybe I'll activate the flag because now GitHub complains, with a big red message, that I can't do that action in this moment.

About the fix, I've added tests for the case BYMONTHDAY>31 that is treated by the rule parser. Moreover, as I mentioned in the above answer to mschroeder, I've added a counter that allows to catch those condictions that hangs the code and cause global dataloss when the BYMONTHDAY doesn't exist for a given BYMONTH (e.g. the 31st in April) and added related tests.
For the third note in the review, "the possibility for an else case" was included in the "if" block above. Now I've changed it and moved that case in the "else" mentioned in the review so the code should be more clear now.
I've also added three tests for yearly rules that have nothing to do with this pull request, but that should be in the tests list.

mschroeder · 2017-08-13T12:56:54Z

@Decathlon1578, can you try to set the flag "Allow edits from maintainers" again, please?

kewisch · 2020-01-13T14:19:23Z

@mschroeder maybe you want to just open a new PR and we'll close this one?

mschroeder · 2020-02-08T20:45:05Z

Opened new PR #418 as suggested.

kewisch requested changes Jan 2, 2017

View reviewed changes

Decathlon1578 force-pushed the monthlyBymonthday branch 2 times, most recently from 8e9f69e to 434b444 Compare July 30, 2017 17:51

Decathlon1578 force-pushed the monthlyBymonthday branch 2 times, most recently from 1e93f1b to 147b97e Compare July 31, 2017 07:02

Decathlon1578 added 2 commits July 31, 2017 10:36

Wrong first occurrence for Monthly rule with BYMONTHDAY in particular…

da017fa

… cases

Fixed issues from review

b0db4e3

Decathlon1578 force-pushed the monthlyBymonthday branch from 147b97e to b0db4e3 Compare July 31, 2017 08:40

grunt package

5079ddf

Decathlon1578 force-pushed the monthlyBymonthday branch from 7a84450 to 5079ddf Compare July 31, 2017 15:38

mschroeder assigned Decathlon1578 Aug 13, 2017

mschroeder added this to the 1.3.0 milestone Aug 13, 2017

kewisch added the needinfo More information has been requested label Jan 13, 2020

mschroeder mentioned this pull request Feb 8, 2020

Wrong first occurrence for Monthly rule with BYMONTHDAY in particular cases #418

Open

mschroeder removed the needinfo More information has been requested label Feb 8, 2020

mschroeder closed this Feb 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrong first occurrence for Monthly rule with BYMONTHDAY in particular cases #232

Wrong first occurrence for Monthly rule with BYMONTHDAY in particular cases #232

Decathlon1578 commented Apr 26, 2016

coveralls commented Apr 26, 2016 •

edited

Loading

kewisch left a comment

kewisch Jan 2, 2017

kewisch Jan 2, 2017

kewisch Jan 2, 2017

mschroeder commented Jun 23, 2017

Decathlon1578 commented Jul 2, 2017

Decathlon1578 commented Jul 30, 2017

mschroeder commented Jul 30, 2017

Decathlon1578 commented Jul 31, 2017

mschroeder commented Aug 13, 2017

kewisch commented Jan 13, 2020

mschroeder commented Feb 8, 2020

Wrong first occurrence for Monthly rule with BYMONTHDAY in particular cases #232

Wrong first occurrence for Monthly rule with BYMONTHDAY in particular cases #232

Conversation

Decathlon1578 commented Apr 26, 2016

coveralls commented Apr 26, 2016 • edited Loading

kewisch left a comment

Choose a reason for hiding this comment

kewisch Jan 2, 2017

Choose a reason for hiding this comment

kewisch Jan 2, 2017

Choose a reason for hiding this comment

kewisch Jan 2, 2017

Choose a reason for hiding this comment

mschroeder commented Jun 23, 2017

Decathlon1578 commented Jul 2, 2017

Decathlon1578 commented Jul 30, 2017

mschroeder commented Jul 30, 2017

Decathlon1578 commented Jul 31, 2017

mschroeder commented Aug 13, 2017

kewisch commented Jan 13, 2020

mschroeder commented Feb 8, 2020

coveralls commented Apr 26, 2016 •

edited

Loading