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

Read and write time zone ID when updating CalDAV availability #29318

Merged
merged 2 commits into from
Oct 25, 2021

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Oct 19, 2021

Tiny bug/limitation of #27466

Now the timezone id is read from the previous value but also encoded in the newly saved icalendar value. If no time zone was set before it will still fall back to the auto detected one.

Bildschirmfoto von 2021-10-19 13-43-56

How to test

  • Open /settings/user/groupware
  • Set the timezone to something other than the auto-detected value
  • Save the availability
  • Reload the page

On master: you see the auto detected time zone again
On this branch: time zone is the one you previously stored

Without this fix reloading the page and saving the availability will shift your times by the drift of the time zone.

I tested this against an availability rule created by Apple Calendar. The only diff to their format is that we don't generate the DST vs standard time recurrence rules within the timezone component but I guess we would need some data for this that we currently don't have. done with the help of https://www.npmjs.com/package/icalzone

@ChristophWurst ChristophWurst added this to the Nextcloud 23 milestone Oct 19, 2021
@ChristophWurst ChristophWurst self-assigned this Oct 19, 2021
@ChristophWurst ChristophWurst requested review from artonge, Pytal and szaimen and removed request for a team October 19, 2021 09:59
@tcitworld

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@tcitworld

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@ChristophWurst ChristophWurst force-pushed the fix/dav-availability-settings-timezone-id branch from d3eeb85 to 6c76e2d Compare October 19, 2021 11:50
@tcitworld
Copy link
Member

The data from https://github.com/touch4it/ical-timezones/tree/master/lib/zones seems to never have been updated since the start of the project, so we'll probably encounter issues soon enough.

@ChristophWurst
Copy link
Member Author

The data from https://github.com/touch4it/ical-timezones/tree/master/lib/zones seems to never have been updated since the start of the project, so we'll probably encounter issues soon enough.

Fair point, but do we have alternatives?

@ChristophWurst
Copy link
Member Author

The data from https://github.com/touch4it/ical-timezones/tree/master/lib/zones seems to never have been updated since the start of the project, so we'll probably encounter issues soon enough.

I ran their generator. I don't see any changes except formatting differences with quoted object key.

@tcitworld
Copy link
Member

tcitworld commented Oct 19, 2021

I ran their generator. I don't see any changes except formatting differences with quoted object key.

That's because it doesn't update the .ics files, only the js files. The VTIMEZONE data was provided by https://github.com/benfortuna/tzurl (according to this).

I've used https://github.com/libical/vzic/ (which seems active) to generate the VTIMEZONE data from latest IANA data but I've yet to use it with touch4it/ical-timezones.

@ChristophWurst
Copy link
Member Author

I've used https://github.com/libical/vzic/ (which seems active) to generate the VTIMEZONE data from latest IANA data but I've yet to use it with touch4it/ical-timezones.

Is there anything I could help with?

@skjnldsv
Copy link
Member

Rebasing? :)

@tcitworld
Copy link
Member

Is there anything I could help with?

Nah, that's non blocking for this PR anyway.

Tiny bug/limitation of #27466

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst force-pushed the fix/dav-availability-settings-timezone-id branch from 6c76e2d to 6301531 Compare October 25, 2021 09:12
@skjnldsv
Copy link
Member

skjnldsv commented Oct 25, 2021

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   apps/dav/js/settings-personal-availability.js
	modified:   apps/dav/js/settings-personal-availability.js.map

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Minor request, otherwise looks good!

@skjnldsv skjnldsv mentioned this pull request Oct 25, 2021
25 tasks
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst force-pushed the fix/dav-availability-settings-timezone-id branch from 6301531 to a34f94d Compare October 25, 2021 13:57
@GretaD
Copy link
Contributor

GretaD commented Oct 25, 2021

tested and it works

@ChristophWurst ChristophWurst merged commit 9177afe into master Oct 25, 2021
@ChristophWurst ChristophWurst deleted the fix/dav-availability-settings-timezone-id branch October 25, 2021 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants