ics import problems due to malformed VTIMEZONE definition

@mandclu Happy new year! Just bumping this to see if you've had time to consider whether you would accept another developer from our team here at the University of Texas at Austin testing and marking this issue as RTBC, as long as we affirm that the review is being done independently?

Allow me to recount a personal anecdote that hopefully will shed light on the revised approach I'm proposing here. In early November, I joined a local photography club, and used an add-to-calendar link on their website to add the weekly meetings to my Google Calendar. Since daylight savings ended, every single meeting is off by an hour, so each week I have to manually update the meeting in my own calendar. I relate this experience because it illustrates that it isn't sufficient to publish times in UTC and trust the calendar software to get it right. Particularly for recurring events that can move in and out of daylight savings time, knowing the proper timezone is invaluable, when this can be communicated without introducing other problems.

As such, I have decided that since the problem reported in this issue is really with Outlook, it isn't advisable to remove the timezone information for Google Calendars, which do support this information. I have provided an updated patch (sorry no interdiff today) that creates a separate set of data for Outlook, specifically excluding the timezone and rendering the data in UTC.

Apologies also for including some code standards fixes in with the changes. I know this isn't best practice but wanted to get them included while I'm editing the files.

Let me know if this updated patch still resolves the original issue, and I'll get it merged in.

@mandclu Thanks for the update, I'll try to get some time to look at the new patch this week.

Would you indulge me in sharing the URL to the calendar for your photography club? Assuming they haven't updated it since you encountered that problem, I would like to look at their ICS file structure to better understand why your calendar behaved the way it did.

I think I originally added their event to my calendar on my phone, so it would be tricky to find the original file. Here's a event export I created today. I had to add a txt extension so drupal.org would let me upload it.

it isn't advisable to remove the timezone information for non-Outlook clients, which do support this information. I have provided an updated patch that creates a separate set of data for Outlook, specifically excluding the timezone and rendering the data in UTC.

I'm in support in principle of the approach above. However, I'm not getting the expected output with patch #24.

Additionally, when I started to set about validating the output through the existing Unit test, I got an error:

1) Drupal\Tests\addtocal_augment\Unit\BasicLinkTest::testCal
Drupal\Core\DependencyInjection\ContainerNotInitializedException: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.

Setting this to "Needs work", with the following remaining work to be done:

1. Establish that the Outlook variant does in fact excluding the timezone information and the iCal variant has no change.
2. Remediate the failing test above.
3. Add test coverage for the Outlook variant.

The attached patch restores the Unit test functioning and adds test coverage for the Outlook variants. Only a small change was made to the business logic, setting the $separator as a class variable so that it could be used in the test.

Setting to "Needs Review"!

Hi @mandclu, just checking in to see if you would be willing to look at the newer patches above from @mark_fullmer? Thanks!

Thanks for everyone's work (and patience) here.

Generally the patch in #28 looks good, but there is one thing I'm not sure I understand about the updated code. In the updated BasicLinkTest :: testCal for both the ical and outlook tests, it does an implodeRecursive() using the addtocal separator and then does an explode on the result for the comparison. Is that to flatten the generated array?

Yes, that scansion of the code intent in the new test coverage is correct. The reason I did this was in order to as faithfully reproduce the way that the actual render proceeds (by doing the implodeRecursive() with its separator) while being able to have a test expectation compare against an actual output as a simple array.

In my testing, comparing two arrays in the test made it much simpler to see what, if anything, was dissimilar about the output.

Merged in and will roll a new release shortly. Thanks again!

Automatically closed - issue fixed for 2 weeks with no activity.

I tried the 1.1.0-beta1 version and the issue is persisting for me where the Outlook link does not open in my Outlook desktop app. The link for Google works and the iCal link opens in the default mail app for Mac.

I am on a Mac Apple M1, on Ventura and Outlook version 16.73.

I tried the 1.1.0-beta1 version and the issue is persisting for me where the Outlook link does not open in my Outlook desktop app.

I can confirm that this is the case. It's due to missing colons on the DSTART and DTEND values. I'll create a new issue.

I've created a new issue and provided a patch at 🐛 [Outlook] DTSTART and DTEND values missing colon Fixed