-
Notifications
You must be signed in to change notification settings - Fork 241
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
Conversation
There was a problem hiding this 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?
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
unassigning milestone because this will go into main |
I need to look into the sabre code more but having the full timezone info could be good to have. |
With https://gist.github.com/thomascube/47ff7d530244c669825736b10877a200 it works fine, for timezones that have offests it also works well. |
There was a problem hiding this 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?
It could make sense to create a new class for the tz generation. That makes testing easier and removes complexity from the appointment code. |
The generator looks great! Let's add some unit tests :) |
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. |
This needs some tests, then we're good to merge. I hope to get this in before the next release on thursday. |
/backport to stable4.5 |
33994a5
to
cf64714
Compare
There was a problem hiding this 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
0597477
to
a4347de
Compare
Signed-off-by: Anna Larch <[email protected]>
d68f9b2
to
496896a
Compare
* * 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤣
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:
After: