Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add VTIMEZONE to Appointments #5339

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

miaulalala
Copy link
Contributor

@miaulalala miaulalala commented Jun 27, 2023

fixes part of #5031

Tested it but this doesn't fix the diverging timezones. Still, it makes us RFC compliant which is always a good thing, and might fix Outlook, although I don't have an Outlook setup to test.

Before:

BEGIN:VCALENDAR
VERSION:2.0
PRODID:-//Sabre//Sabre VObject 4.5.3//EN
CALSCALE:GREGORIAN
BEGIN:VEVENT
UID:sabre-vobject-4d193331-955f-4041-a7b9-44b14809b030
DTSTAMP:20230914T191309Z
SUMMARY:Bob - Have Lunch With Me
STATUS:CONFIRMED
DTSTART;TZID=Europe/Vienna:20230918T120000
DTEND;TZID=Europe/Vienna:20230918T123000
DESCRIPTION:Testing
ORGANIZER;CN=Anna Admin;CUTYPE=INDIVIDUAL;PARTSTAT=ACCEPTED:mailto:xxxxx
 [email protected]
ATTENDEE;CN=Anna Admin;CUTYPE=INDIVIDUAL;RSVP=TRUE;ROLE=REQ-PARTICIPANT;PAR
 TSTAT=ACCEPTED:mailto:[email protected]
ATTENDEE;CN=Bob;CUTYPE=INDIVIDUAL;RSVP=TRUE;ROLE=REQ-PARTICIPANT;PARTSTAT=A
 CCEPTED:mailto:[email protected]
LOCATION:Abigail's Kitchen
X-NC-APPOINTMENT:iHFJtxxcrW8g
END:VEVENT
END:VCALENDAR

After:

BEGIN:VCALENDAR
VERSION:2.0
PRODID:-//Sabre//Sabre VObject 4.5.3//EN
CALSCALE:GREGORIAN
BEGIN:VTIMEZONE
TZID:Europe/Vienna
BEGIN:DAYLIGHT
DTSTART:20230326T010000
TZOFFSETFROM:+0100
TZOFFSETTO:+0200
TZNAME:CEST
END:DAYLIGHT
BEGIN:DAYLIGHT
DTSTART:20240331T010000
TZOFFSETFROM:+0100
TZOFFSETTO:+0200
TZNAME:CEST
END:DAYLIGHT
BEGIN:STANDARD
DTSTART:20231029T010000
TZOFFSETFROM:+0200
TZOFFSETTO:+0100
TZNAME:CET
END:STANDARD
END:VTIMEZONE
BEGIN:VEVENT
UID:sabre-vobject-27752eed-efcb-4972-8cd5-84f78b65c6ae
DTSTAMP:20231102T134259Z
SUMMARY:Test - Have Lunch With Me
STATUS:CONFIRMED
DTSTART;TZID=Europe/Vienna:20231106T120000
DTEND;TZID=Europe/Vienna:20231106T123000
DESCRIPTION:Testing
ORGANIZER;CN=Anna Admin;CUTYPE=INDIVIDUAL;PARTSTAT=ACCEPTED:mailto:[email protected]
ATTENDEE;CN=Anna Admin;CUTYPE=INDIVIDUAL;RSVP=TRUE;ROLE=REQ-PARTICIPANT;PAR
 TSTAT=ACCEPTED:mailto:[email protected]
ATTENDEE;CN=Test;CUTYPE=INDIVIDUAL;RSVP=TRUE;ROLE=REQ-PARTICIPANT;PARTSTAT=
 ACCEPTED:mailto:[email protected]
LOCATION:Abigail's Kitchen
X-NC-APPOINTMENT:iHFJtxxcrW8g
END:VEVENT
END:VCALENDAR

@miaulalala miaulalala added 2. developing Work in progress bug labels Jun 27, 2023
@miaulalala miaulalala added this to the v4.4.2 milestone Jun 27, 2023
@miaulalala miaulalala self-assigned this Jun 27, 2023
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it sufficient to only have TZID or do we need the full tz spec?

@ChristophWurst ChristophWurst removed this from the v4.4.2 milestone Jun 27, 2023
@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (33e236c) 24.43% compared to head (496896a) 24.67%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5339      +/-   ##
============================================
+ Coverage     24.43%   24.67%   +0.24%     
- Complexity      388      406      +18     
============================================
  Files           240      241       +1     
  Lines         10643    10693      +50     
  Branches       1739     1739              
============================================
+ Hits           2601     2639      +38     
- Misses         8042     8054      +12     
Flag Coverage Δ
javascript 15.67% <ø> (ø)
php 62.36% <ø> (+0.33%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChristophWurst
Copy link
Member

unassigning milestone because this will go into main

@miaulalala
Copy link
Contributor Author

Is it sufficient to only have TZID or do we need the full tz spec?

I need to look into the sabre code more but having the full timezone info could be good to have.

@miaulalala
Copy link
Contributor Author

With https://gist.github.com/thomascube/47ff7d530244c669825736b10877a200 it works fine, for timezones that have offests it also works well.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have a test or two for the new timezone generation?

lib/Service/Appointments/BookingCalendarWriter.php Outdated Show resolved Hide resolved
@ChristophWurst
Copy link
Member

It could make sense to create a new class for the tz generation. That makes testing easier and removes complexity from the appointment code.

@miaulalala miaulalala marked this pull request as draft July 20, 2023 16:08
@ChristophWurst
Copy link
Member

The generator looks great! Let's add some unit tests :)

@toabi
Copy link

toabi commented Oct 27, 2023

Hi, what's the status about this? We have lots of issues with managing events with Customers using outlook… they don't really read text and just trust their Outlook Events. Which are currently offset by 1/2 hours… probably because of this bug.

@miaulalala
Copy link
Contributor Author

This needs some tests, then we're good to merge. I hope to get this in before the next release on thursday.

@miaulalala
Copy link
Contributor Author

/backport to stable4.5

@backportbot-nextcloud backportbot-nextcloud bot added the backport-request A backport was requested for this pull request label Oct 30, 2023
@miaulalala miaulalala marked this pull request as ready for review November 2, 2023 13:29
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. Extraneous classes will byte us when they change

Comment on lines +17 to +36
* * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* * GNU AFFERO GENERAL PUBLIC LICENSE for more details.
* *
* * You should have received a copy of the GNU Affero General Public
* * License along with this library. If not, see <http://www.gnu.org/licenses/>.
* *
*
*/

declare(strict_types=1);

/*
* @copyright 2023 Anna Larch <[email protected]>
*
* @author 2023 Anna Larch <[email protected]>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this what they call dual licensing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤣

@ChristophWurst ChristophWurst merged commit 27c4402 into main Nov 7, 2023
41 checks passed
@ChristophWurst ChristophWurst deleted the fix/add-tz-to-appointment-ics branch November 7, 2023 13:49
@backportbot-nextcloud backportbot-nextcloud bot removed the backport-request A backport was requested for this pull request label Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
Development

Successfully merging this pull request may close these issues.

4 participants