Use timezone ID instead of full VTIMEZONE in DB #1104
Conversation
Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
rfc2822
force-pushed
the
1052-use-timezone-id-instead-of-full-vtimezone-in-db
branch
from
497236a
to
b4b7bad
Compare
|
Is there something missing for this PR yet? |
It should be ready, I forgot to mark it as complete 😅 |
There was a problem hiding this comment.
Really nice PR! A pleasure to review ;)
Can you tell me how to test the migration? I can see someone created a "Calendar with timezone" in the nextcloud test instance, but it does not have a timezone in the collections table in the database? None of the calendars does in fact, they are all NULL.
I tried to figure out how to change the collection/calendar timezone in the nextcloud interface but neither the setting in the calendar view, nor the "locale" setting for the user seems to work ...
How did you do it?
| @@ -202,6 +206,10 @@ data class Collection( | |||
| } | |||
| } | |||
|
|
|||
| if (timezoneId == null && timezone != null) { | |||
There was a problem hiding this comment.
It looks like this does not update the timezoneId when it exists already. Is this what we want? Would we not always want to replace/update it, in case it changes on the server?
| @@ -112,6 +113,26 @@ abstract class AppDatabase: RoomDatabase() { | |||
| // manual migrations | |||
|
|
|||
| val migrations: Array<Migration> = arrayOf( | |||
| object : Migration(14, 15) { | |||
| override fun migrate(db: SupportSQLiteDatabase) { | |||
| // the timezone column has been removed, now it's timezoneId | |||
There was a problem hiding this comment.
Maybe make this a complete sentence, capitalized and punctuated or add an empty line after this one. One might think the sentence continues on the next line otherwise (I did at least).
Purpose
Currently we are storing the full
VTimeZonedefinition in the database, even though we only use the timezone ID.This PR aims to fix this, and only store the identifier.
Short description
timezonefromCollectiontimezoneIdtoCollection1514to15:timezoneIdcolumn.timezone, and copy it totimezoneId(if not null)timezonecolumn.Note
We are storing the timezone id as given by the server, which may not be a valid Android Timezone. This is obviously converted later on when storing into the calendar provider, but maybe it's not a bad idea to convert it beforehand.
More information
calendar-timezone-idis already being used forMKCALENDARsince MKCALENDAR: send VTIMEZONE in calendar-timezone #1044dav4jvmgot support forCalendarTimezoneIdat bitfireAT/dav4jvm@fbd95a5calendar-timezonemust contain a fullCALENDARas speciifed by Calendaring Extensions to WebDAV (CalDAV) Section 5.2.2calendar-timezone-iddefinition: CalDAV: Time Zones by Reference - Section 5.2Checklist