fix(caldav): Incoming tasks do not get correct time zone #1455

ce72 · 2023-04-02T11:53:03Z

ce72 commented

2023-04-02 11:53:03 +00:00

Dates from tasks.org may be formatted like DUE;TZID=Europe/Berlin:20230402T150000
After this fix the parameter TZID is no longer ignored and the Vikunja task gets a DueDate of 13:00 UTC, which corresponds to 15:00 in Europe/Berlin. Before this fix, the time was parsed to 15:00 UTC.

Resolves vikunja/api#1453

Dates from tasks.org may be formatted like DUE;TZID=Europe/Berlin:20230402T150000 After this fix the parameter TZID is no longer ignored and the Vikunja task gets a DueDate of 13:00 UTC, which corresponds to 15:00 in Europe/Berlin. Before this fix, the time was parsed to 15:00 UTC. Resolves https://kolaente.dev/vikunja/api/issues/1453

🎉 1

konrad reviewed 2023-04-02 12:13:03 +00:00

pkg/caldav/parsing.go Outdated

						
				@ -218,0 +222,4 @@

					if len(tzParameter) > 0 {

						loc, err := time.LoadLocation(tzParameter[0])

						if err != nil {

							log.Warningf("Error while parsing caldav timezone %s: %s", tzParameter[0], err)

konrad commented

2023-04-02 12:13:00 +00:00

I think we should return when an error happens. Or put differently, what should happen when this fails? Should it just continue?

ce72 commented

2023-04-02 12:25:34 +00:00

My idea was to be tolerant. If the location string is unknown, we still parse without location in line 236. This will then return the same (invalid) time like the code in main.
Do you really think we should change that?

My idea was to be tolerant. If the location string is unknown, we still parse without location in line 236. This will then return the same (invalid) time like the code in main. Do you really think we should change that?

konrad commented

2023-04-02 12:27:50 +00:00

That explanation makes sense. Fine to leave it like that.

konrad marked this conversation as resolved

pkg/caldav/parsing_test.go Outdated

						
				@ -222,0 +276,4 @@

										Title: "Vikunja",

									},

								},

								DueDate:   time.Date(2023, 4, 2, 15, 0, 1, 0, config.GetTimeZone()),

konrad commented

2023-04-02 12:11:33 +00:00

Shouldn't this be Europe/Berlin and `17:00 instead of the configured time zone?

Shouldn't this be `Europe/Berlin` and `17:00 instead of the configured time zone?

ce72 commented

2023-04-02 12:26:32 +00:00

How does Vikunja store timezones? I was not sure about this.
In line 231 I convert the parsed time (which in the test is in CEST) to the configured timezone, to be safe. Do you think I should omit this? Can we store times at any timezone?

How does Vikunja store timezones? I was not sure about this. In line 231 I convert the parsed time (which in the test is in CEST) to the configured timezone, to be safe. Do you think I should omit this? Can we store times at any timezone?

konrad commented

2023-04-02 12:30:44 +00:00

How does Vikunja store timezones?

It stores everything in utc in the database, then lets the users configure their time zone for things like reminders and other emails.

I think it's okay to save it in the configured time zone and then check that here, the parsed time result here might be different if people run the tests with a different time zone configured. That's an edge case though and something we can leave to the people doing that.

> How does Vikunja store timezones? It stores everything in utc in the database, then lets the users configure their time zone for things like reminders and other emails. I think it's okay to save it in the configured time zone and then check that here, the parsed time result here might be different if people run the tests with a different time zone configured. That's an edge case though and something we can leave to the people doing that.

ce72 commented

2023-04-02 13:09:42 +00:00

Understand. But then I think line 231 should better be

t = t.In(time.UTC)

If other times are stored in UTC, I didn't mean to deviate. Maybe you can fix this if you come along. But, as you say, both versions lead to the same result and both pass the test.

Understand. But then I think [line 231](https://kolaente.dev/vikunja/api/src/branch/main/pkg/caldav/parsing.go#L231) should better be ``` t = t.In(time.UTC) ``` If other times are stored in UTC, I didn't mean to deviate. Maybe you can fix this if you come along. But, as you say, both versions lead to the same result and both pass the test.

👍 1

konrad marked this conversation as resolved

ce72 force-pushed main from f93a2855ed to 300100ad8a

2023-04-02 12:14:04 +00:00

Compare

konrad approved these changes 2023-04-02 12:31:18 +00:00

konrad merged commit 1cffef6908 into main

2023-04-02 12:31:33 +00:00

konrad referenced this issue from a commit

2023-04-02 12:31:33 +00:00

fix(caldav): Incoming tasks do not get correct time zone (#1455)

konrad commented