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

Add support for daylight saving time #84

Closed
damithc opened this issue May 2, 2014 · 32 comments · Fixed by #8687
Closed

Add support for daylight saving time #84

damithc opened this issue May 2, 2014 · 32 comments · Fixed by #8687
Assignees
Labels
committers only Difficult; better left for committers or more senior developers p.Medium Marginal impact; would like to do if time permits
Milestone

Comments

@damithc
Copy link
Contributor

damithc commented May 2, 2014

From dam...@gmail.com on June 30, 2011 16:11:15

Does time zone support work correctly when day light saving is involved?

Original issue: http://code.google.com/p/teammatespes/issues/detail?id=85

@damithc
Copy link
Contributor Author

damithc commented May 2, 2014

From dam...@gmail.com on December 05, 2011 23:53:36

Labels: Priority-Medium

@damithc
Copy link
Contributor Author

damithc commented May 2, 2014

From dam...@gmail.com on July 12, 2012 22:49:22

Status: Accepted

@damithc
Copy link
Contributor Author

damithc commented May 2, 2014

From dam...@gmail.com on February 20, 2013 04:15:26

Labels: Type-Task Difficulty-High

@damithc
Copy link
Contributor Author

damithc commented May 2, 2014

From modsj...@gmail.com on February 21, 2013 09:17:45

Hi Dr. Damith,

I am not sure how teammates is implementing the timezone change for daylight savings however, to make sure that it is working correctly, it will be good to use the class SimpleTimeZone http://docs.oracle.com/javase/6/docs/api/java/util/SimpleTimeZone.html

@damithc
Copy link
Contributor Author

damithc commented May 2, 2014

From dam...@gmail.com on February 22, 2013 04:39:47

Thanks for the pointer Allan. Whoever doing this issue should look into it. As of now, I don't think we are handling it all.

@damithc damithc changed the title Does time zone support work correctly when day light saving is involved? Add support for daylight saving time Jan 27, 2015
@damithc damithc added f.Sessions p.Low Very little impact; unlikely to do in the near future and removed t.Task labels Jan 27, 2015
@damithc damithc removed the s.Accepted label Feb 6, 2016
@damithc
Copy link
Contributor Author

damithc commented Feb 6, 2016

@ashrayjain assigning to you, as discussed.

@ashrayjain
Copy link
Contributor

@damithc Hey prof, it turns out we have two more fields (session visible from and responses visible from), apart from session start and session end. Our discussed solution of adding 2 new columns in the db for session start offset and session end offset will not be enough for this. We could solve this problem in the following 3 ways:

  1. We could simply add 2 more columns for the new fields. However, if we ever need to add any more date related info to the Session object, we will need to add new columns for DST as well.
  2. We could store the timezone ID string (like Asia/Singapore or Australia/Adelaide) in our db and remove all other offset related columns (like timeZone, timeZoneDouble as well as all the dst offset columns). Then we will need to update all code that uses the timeZone fields to use Java's standard library and apply this timezone ID (which includes DST info) when retrieving timestamps from the db.
  3. We could do away with storing timezone info separately and change all date fields in our db to have the correct timezone (eg. SGT) as part of the timestamp as opposed to the current value (UTC).

An important consideration would be on how we will handle transitioning for existing sessions. What do you think?

@damithc
Copy link
Contributor Author

damithc commented Feb 15, 2016

It has to be 2 or 3. What is the common wisdom on handling timezones and DST? Any articles on this?

@ashrayjain
Copy link
Contributor

There are a lot of different opinions on StackOverflow about this, and a lot of times they are for specific cases. The best article that I think we should follow is this one.
https://www.w3.org/TR/timezone/#guidelinessummary

On the same article, these sections are also of interest:
https://www.w3.org/TR/timezone/#guidelinesfuturerecurring
https://www.w3.org/TR/timezone/#negotiating

The general overview is that, especially when dates in the future are involved, storing the timezone ID is the best way. Other things can be used as fallbacks but timezone IDs should be used if available.

@damithc
Copy link
Contributor Author

damithc commented Feb 15, 2016

I'm ok with storing timezone ID. We also need to discuss where to store it. For example, may be we should store in in the Course object rather than the Session object? Explore these further and we can discuss on Wednesday.

@damithc
Copy link
Contributor Author

damithc commented Feb 15, 2016

You also need to figure out how to save DST lookup table and how to keep it updated.

@ashrayjain
Copy link
Contributor

Since we will be using the standard library for Java for DST conversions, we will need to update the standard library's data. The common method for updating this data is outlined here.
http://www.oracle.com/technetwork/java/javase/tzupdater-readme-136440.html

However, I will need to look into how it would work with GAE.

@ashrayjain
Copy link
Contributor

With GAE, the standard library is locked in and out of our control. This is the only issue about this I could find.

Google does seem to update its timezone data but not very frequently as it turns out. I tested the standard library on a GAE test app using known timezone/dst updates from the past year (eg. North Korea's timezone update from August 2015). From what I tested, updates until June last year are in. I would guess that Google follows a yearly update cycle for this. So the question is, if this frequency is ok for us?

The other alternative is to use an external library instead of the standard library so that we can update the data as we please. The most popular alternative seems to be Joda Time.

@damithc
Copy link
Contributor Author

damithc commented Feb 16, 2016

Yes, I'm ok to use an external library as Java built-in date library is known to be weak. Is Joda time updated more regularly?

@ashrayjain
Copy link
Contributor

Yup sir. Joda seems to follow the updates to the database pretty closely.
Their latest version already includes the first updates for 2016 released
in late January. So should I go ahead with it?

@damithc
Copy link
Contributor Author

damithc commented Feb 16, 2016

Yes, go ahead with Joda.

@ashrayjain
Copy link
Contributor

@damithc Since we are adding timezone info to courses, I wanted to clarify some semantics. It looks like once a course is created, we don't allow the instructor to change the Course ID or Course Name (am i right?). As we will be adding the timezone option there, do we also disallow timezone updates after the course has been created? Is there any use case for timezone updates after course creation?

Note: We will still allow the user to change the detected value on the course create page, BEFORE the course is created.

@ashrayjain
Copy link
Contributor

@damithc bump.

@damithc
Copy link
Contributor Author

damithc commented Feb 21, 2016

No reason not to allow it right?
The other two fields should be allowed to change too. It's just that we didn't find time to do it.

@ashrayjain
Copy link
Contributor

Hmm. I don't see any reason to disallow changing course name and timezone. But maybe ID is more sensitive to change? @thyageshm your thoughts?

In any case, I think it will be better to add this functionality as a separate issue rather than with this one?

@damithc
Copy link
Contributor Author

damithc commented Feb 21, 2016

Yes, editability of each field can be separate issues. Yes, editing course ID is the trickiest and may never be allowed.

@whipermr5
Copy link
Member

whipermr5 commented Jul 18, 2017

From what I understand, we have to:

  • Migrate FeedbackSession timeZone field from double to string containing time zone ID.
    • We can use the Etc/GMT-X time zones to maintain expected behaviour, as these time zones do not have DST. It is up to instructors to change the time zone to their correct local time zone if they want to enable DST.
  • Migrate timestamp data within FeedbackSession such that timestamps refer to the actual time in UTC rather than local time.
    • Fields include startTime, endTime, sessionVisibleFromTime, resultsVisibleFromTime.
    • Currently they are interpreted as local time. There is a dependency on session timeZone to interpret the time correctly.
  • Update all views of the time fields to interpret the timestamps as actual time in UTC, and pretty print them in the time zone specified by the timeZone field using a date library that handles DST.

Is there anything I'm missing?

@damithc
Copy link
Contributor Author

damithc commented Jul 18, 2017

@wkurniawan07 can weigh in. He was the last person to handle this hot potato 😝

@wkurniawan07
Copy link
Member

wkurniawan07 commented Jul 18, 2017

The plan was to ditch timezone fields in feedback sessions completely and relying fully on the one in course, which already is in Olson timezone ID. The timezone field in the courses was actually added for this purpose.
I tried and met many roadblocks along the way, and realized one part of the puzzle is #6300, although evidently I did not manage to find the time to actually work on it 😛
After cleaning up all the APIs, the last part of the puzzle should be removing the timezone fields completely from the feedback session and relying on the course's.

For the reason that it has not been actually tried out, I cannot give a 100%-guarantee that the previous plan would actually work, although it sounds plausible on paper. Migrating the timezone field in feedback session would (edit: might) also work, but will require another migration process. Since I was not actually involved in the discussion as for why use timezone in course vs use timezone in sessions, I'd open this topic back to square one again.

@whipermr5
Copy link
Member

Well I guess since our users have gotten used to a session time zone, we shouldn't remove it immediately. We can first migrate the session time zone field, announce support for DST, then deprecate the use of a session specific time zone before finally removing it once we're sure no one uses it anymore?

@damithc
Copy link
Contributor Author

damithc commented Jul 18, 2017

From what I remember, we planned to show the time zone in the session (but readonly) even if it is stored in the course. That way, users will not notice a big difference UI-wise.

@wkurniawan07
Copy link
Member

@whipermr5 I forgot to link this issue as well: #5928

@whipermr5
Copy link
Member

Linking to this issue which contains some discussion: #8362

@whipermr5
Copy link
Member

whipermr5 commented Mar 19, 2018

As discussed with @tran-tien-dat, we should not worry about local date times that are ambiguous or non-existent but leave it to the library to resolve. The javadoc for LocalDateTime#atZone explains clearly how these are resolved.

However I think we should still show a message to the user if the local date time they input is ambiguous or non-existent, stating clearly which time we resolved it to and, in the ambiguous case, allow a way to select the later offset time. Useful methods: ZonedDateTime.ofStrict(LocalDateTime, ZoneOffset, ZoneId) , ZonedDateTime.withLaterOffsetAtOverlap().

@whipermr5
Copy link
Member

Sneak peek:
screen shot 2018-03-20 at 3 55 57 am

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
committers only Difficult; better left for committers or more senior developers p.Medium Marginal impact; would like to do if time permits
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants