Fix issue #3250: Respect deleted (excluded) calendar events

[jkriegshauser]

Hello and thank you for wanting to contribute to the MagicMirror² project

Please make sure that you have followed these 4 rules before submitting your Pull Request:

1. Base your pull requests against the develop branch.
2. Include these infos in the description:

• Does the pull request solve a related issue?
• If so, can you reference the issue like this Fixes #<issue_number>?
• What does the pull request accomplish? Use a list if needed.
• If it includes major visual changes please add screenshots.

3. Please run npm run lint:prettier before submitting so that
   style issues are fixed.
4. Don't forget to add an entry about your changes to
   the CHANGELOG.md file.

Note: Sometimes the development moves very fast. It is highly
recommended that you update your branch of develop before creating a
pull request to send us your changes. This makes everyone's lives
easier (including yours) and helps us out on the development team.

Thanks again and have a nice day!

[rejas]

Thx for the PR. COuld you also write a test for this?

[jkriegshauser]

@rejas After much ado, the unit test is added. I verified locally that it failed without the fix commit. Also needed to fix up .prettierignore since it seems like no one has added any .ics files since the switch to using prettier (git commit hook was failing)

[rejas]

True, the ics should be added to prettierignore, good catch. But why add the ignore files too?

[jkriegshauser]

True, the ics should be added to prettierignore, good catch. But why add the ignore files too?

Oddly enough, .prettierignore didn't ignore itself, so Prettier complained that it couldn't find a linter for .prettierignore. Originally I had also made some changes to .eslintignore as well, but ended up not needing them. The same issue will exist the next time it is changed though. Basically we're going to have an issue with any file type that Prettier doesn't understand as soon as someone tries to commit that type.

[jkriegshauser]

@rejas Do I need to do anything with the failing tests? It looks like they're running current tests on old versions of MM which would of course break as they don't have the fix

[rejas]

@rejas Do I need to do anything with the failing tests? It looks like they're running current tests on old versions of MM which would of course break as they don't have the fix

No, the tests should be running on the PR you made, so are you sure they are running fine on our local machine?

[jkriegshauser]

@rejas yep, here's the final output from me running npm run test:e2e locally:

Test Suites: 22 passed, 22 total
Tests:       430 passed, 430 total
Snapshots:   0 total
Time:        126.971 s
Ran all test suites.

[rejas]

What node version did you use when running the tests?

[jkriegshauser]

~/dev/externals/MagicMirror$ npm --version
10.1.0
~/dev/externals/MagicMirror$ node --version
v20.9.0

[khassel]

when running the e2e tests on your branch I'm getting this error:

Summary of all failing tests
 FAIL  tests/e2e/modules/calendar_spec.js (65.054 s)
  ● Calendar module › exdate check › should show the recurring event 51 times (excluded once) in a 364-day (inclusive) period

    expect(received).toBe(expected) // Object.is equality

    Expected: 51
    Received: 52

      14 |                      expect(elem.length).not.toBe(result);
      15 |              } else {
    > 16 |                      expect(elem.length).toBe(result);
         |                                          ^
      17 |              }
      18 |      };
      19 |

      at toBe (tests/e2e/modules/calendar_spec.js:16:24)
      at Object.<anonymous> (tests/e2e/modules/calendar_spec.js:94:4)


Test Suites: 1 failed, 21 passed, 22 total
Tests:       1 failed, 429 passed, 430 total
Snapshots:   0 total
Time:        136.612 s
Ran all test suites.

node@5cd4b2fb6e3b:/opt/magic_mirror$ git remote -v
origin  https://github.com/jkriegshauser/MagicMirror.git (fetch)
origin  https://github.com/jkriegshauser/MagicMirror.git (push)
node@5cd4b2fb6e3b:/opt/magic_mirror$ git status
On branch issue-3250-fix
Your branch is up to date with 'origin/issue-3250-fix'.

nothing to commit, working tree clean
node@5cd4b2fb6e3b:/opt/magic_mirror$ node -v
v20.8.1

same result with node v21.0.0

[jkriegshauser]

@khassel thanks for testing. What timezone are you in? I suspect it has something to do with different tz.

[khassel]

@jkriegshauser

node@0d036a3068da:/opt/magic_mirror$ echo $TZ
Europe/Berlin
node@0d036a3068da:/opt/magic_mirror$ date
Sat Oct 28 22:34:25 CEST 2023

[jkriegshauser]

Confirmed that it fails for me if I run:

TZ=Europe/Berlin npm run test:e2e

I suspect it will work for you if you run:

TZ=America/Los_Angeles npm run test:e2e

I'll debug it later. I believe that there is a bug deep within rrule that necessitates the date adjustment in calendarfetcherutils.js starting on line 327 (in this PR branch)

[sdetweil]

rrule is a mess... it expects ONLY local date/time.. no timezone.

[khassel]

TZ=America/Los_Angeles npm run test:e2e

works.

If there is no better solution we could create the used ics file on the fly with current timezone (or replace it in the file before testing)

[jkriegshauser]

Can someone kick off the workflows to see if tests pass?

[khassel]

Can someone kick off the workflows to see if tests pass?

done, they fail but this seems unrelated to your changes, your test passed.

I don't think this is a solution because the tests should not depend on a timezone (e.g. we don't know in which timezone the github job will run when doing the tests and everyone should be able to run the tests without setting a timezone ...)

[jkriegshauser]

Looks like the new exdate test is passing now but a different unrelated test is failing:

Summary of all failing tests
FAIL tests/unit/modules/default/utils_spec.js
  ● Default modules utils tests › formatTime › should convert correctly into another timezone

    expect(received).toBe(expected) // Object.is equality

    Expected: "07:13"
    Received: "08:13"

      161 | 					time
      162 | 				)
    > 163 | 			).toBe("07:13");
          | 			  ^
      164 | 		});
      165 | 	});
      166 | });

      at Object.toBe (tests/unit/modules/default/utils_spec.js:163:6)


Test Suites: 1 failed, 39 passed, 40 total
Tests:       1 failed, 572 passed, 573 total
Snapshots:   9 passed, 9 total
Time:        146.051 s
Ran all test suites in 3 projects.
Force exiting Jest: Have you considered using `--detectOpenHandles` to detect async operations that kept running after all tests finished?
Error: Process completed with exit code 1.

[jkriegshauser]

I don't think this is a solution because the tests should not depend on a timezone (e.g. we don't know in which timezone the github job will run when doing the tests and everyone should be able to run the tests without setting a timezone ...)

In general I agree with this sentiment. I think the underlying problem is in the rrule module that node-ical uses, and fixing that is a huge undertaking. It looks like there's a comment:

// not full day, but luxon can still screw up the date on the rule processing
// we need to correct the date to get back to the right event for

In other words, there's already some hackery around trying to correct the date because the underlying recurrence rule calculation is faulty, which seems to be coming from rule.between(). I'm just fixing it so that dateKey is calculated on the adjusted date, not the previous incorrect date (which causes excluded recurrences from not being correctly found).

[khassel]

I'll try to investigate this over the next few days. The next release is 2024, so we have some time. I think we've had similar issues with other tests, but I'll have to search to find this ... and in my time zone it's too late for that now ;)

[sdetweil]

rrule is not the tool to use but it's the one behing used

rrule depends on local date and time not tz based datetime.

but node-ical parses the ICS into tz based dates and times.. BUT WE DONT KNOW because we never process the ICS ourselves.

I have spent a few months looking at this problem with an issue open on node-ical. but I cannot find a reliable fix

[rejas]

you closed this @sdetweil ?

[sdetweil]

sorry, errant finger check

[khassel]

I did some tests and changing the content of the ics-file from Europe/Berlin to UTC seems to work.

I looped over different timezones and the test always passed

'Pacific/Niue',
'Pacific/Rarotonga',
'Pacific/Marquesas',
'Pacific/Gambier',
'America/Juneau',
'America/Creston',
'America/Belize',
'America/Eirunepe',
'America/Antigua',
'Antarctica/Palmer',
'America/St_Johns',
'America/Noronha',
'Atlantic/Cape_Verde',
'Antarctica/Troll',
'Europe/Berlin',
'Africa/Gaborone',
'Antarctica/Syowa',
'Asia/Tehran',
'America/Anguilla',
'Asia/Kabul',
'Antarctica/Mawson',
'Asia/Colombo',
'Asia/Kathmandu',
'Antarctica/Vostok',
'Indian/Cocos',
'Antarctica/Davis',
'Australia/Perth',
'Australia/Eucla',
'Australia/Darwin',
'Antarctica/DumontDUrville',
'Australia/Adelaide',
'Pacific/Pago_Pago',
'Pacific/Fiji',
'Antarctica/McMurdo',
'Pacific/Chatham',
'Pacific/Kiritimati',

so can you please change this again in your PR and merge new develop branch into this PR (because conflict in changelog.md and because unit test is fixed there), thanks!

[jkriegshauser]

@khassel thanks for investigating. Done and done.

[khassel]

@rejas from my side this can be merged now