Migrate feedback session timeZone field to ZoneId #8638 #8639

whipermr5 · 2018-03-14T11:17:42Z

~~Based on #8637. Commits start from 9b921d0.~~

Script used to migrate JSON data:

import json
import os, io
from collections import OrderedDict

def to_utc_format(offset):
    if offset == 0:
        return 'UTC'
    hours = int(offset)
    minutes = int(abs(offset - hours) * 60)
    return 'UTC%+03d:%02d' % (hours, minutes)

filenames = [f for f in os.listdir('.') if os.path.isfile(f) and f.endswith('.json')]

for filename in filenames:
    print(filename)
    with open(filename) as file:
        data = json.loads(file.read(), object_pairs_hook=OrderedDict)

    try:
        if not data.get('feedbackSessions'):
            continue
    except:
        continue

    for fsKey, fs in data.get('feedbackSessions').iteritems():
        for key, val in fs.iteritems():
            if key in ['timeZone']:
                fs[key] = to_utc_format(fs[key])

    with io.open(filename, 'w', encoding='utf8') as file:
        file.write(json.dumps(data, indent=2, separators=(',', ': '), ensure_ascii=False) + '\n')

tran-tien-dat

Most of my comments are about this one thing: I think we should use something like ZoneId.of("+08:00") in instead of TimeHelper.convertToZoneId, as the later should be removed eventually once we finished migrating our UI and Database to ZoneId eventually.

tran-tien-dat · 2018-03-14T14:52:32Z

src/main/java/teammates/storage/entity/FeedbackSession.java

@@ -170,7 +168,7 @@ private void setTimeZoneFromOffsetIfRequired() {
        } else {
            offset = Double.valueOf(timeZone);
        }
-        timeZone = convertOffsetToZoneId(offset);
+        timeZone = TimeHelper.convertToZoneId(offset).getId();


Are isTimeStoredInUtc and timeZoneDouble ready to be removed?

Not yet; still pending a migration script (#8448) but it's not that high priority.

tran-tien-dat · 2018-03-14T14:54:58Z

src/test/java/teammates/test/cases/logic/EmailGeneratorTest.java

        email = new EmailGenerator().generateFeedbackSubmissionConfirmationEmailForStudent(session, student1, time);
        subject = String.format(EmailType.FEEDBACK_SUBMISSION_CONFIRMATION.getSubject(), course.getName(),
                                session.getFeedbackSessionName());
        verifyEmail(email, student1.email, subject, "/sessionSubmissionConfirmationEmailPositiveTimeZone.html");

-        setTimeZoneButMaintainLocalDate(session, -9.5);
+        setTimeZoneButMaintainLocalDate(session, TimeHelper.convertToZoneId(-9.5));


I think you should generate a ZoneId directly, i.e. something like "-09:30". convertToZoneId is just a temporary method, eventually we want to remove all instances of double timezones

tran-tien-dat · 2018-03-14T15:09:23Z

src/test/java/teammates/test/cases/browsertests/InstructorFeedbackSessionsPageUiTest.java

@@ -50,7 +51,7 @@ protected void prepareTestData() {
                .withInstructions(new Text("Please fill in the new feedback session."))
                .withSentOpenEmail(false)
                .withSentPublishedEmail(false)
-                .withTimeZone(8.0)
+                .withTimeZone(TimeHelper.convertToZoneId(8))


Just use something like ZoneId.of("+08:00")? convertToZoneId should be removed eventually.

tran-tien-dat · 2018-03-14T15:09:38Z

src/test/java/teammates/test/cases/browsertests/InstructorFeedbackSessionsPageUiTest.java

@@ -295,7 +296,7 @@ private void testAddAction() throws Exception {

        newSession.setFeedbackSessionName("Allow Early Viewing Session #");
        newSession.setCourseId("CFeedbackUiT.CS1101");
-        newSession.setTimeZone(-4.5);
+        newSession.setTimeZone(TimeHelper.convertToZoneId(-4.5));


Just use something like ZoneId.of("-04:30")? convertToZoneId should be removed eventually.

tran-tien-dat · 2018-03-14T15:09:55Z

src/test/java/teammates/test/cases/browsertests/InstructorFeedbackSessionsPageUiTest.java

@@ -342,7 +343,7 @@ private void testAddAction() throws Exception {
        newSession.setResultsVisibleFromTime(Const.TIME_REPRESENTS_NEVER);
        newSession.setGracePeriod(25);
        newSession.setInstructions(instructions);
-        newSession.setTimeZone(-2);
+        newSession.setTimeZone(TimeHelper.convertToZoneId(-2));


Just use something like ZoneId.of("-02:00")? convertToZoneId should be removed eventually.

tran-tien-dat · 2018-03-14T15:10:32Z

src/test/java/teammates/test/cases/browsertests/InstructorFeedbackSessionsPageUiTest.java

@@ -946,7 +947,7 @@ private void testValidationReload() throws Exception {
                TimeHelper.parseLocalDateTime("01/04/2035", "10", "00"),
                TimeHelper.parseLocalDateTime("30/04/2035", "22", "00"),
                null, null,
-                newSession.getInstructions(), newSession.getGracePeriod(), -2.0);
+                newSession.getInstructions(), newSession.getGracePeriod(), TimeHelper.convertToZoneId(-2));


Just use something like ZoneId.of("-02:00")? convertToZoneId should be removed eventually.

tran-tien-dat · 2018-03-14T15:11:37Z

src/test/java/teammates/test/cases/browsertests/StudentHomePageUiTest.java

@@ -42,7 +41,8 @@ protected void prepareTestData() {
        FeedbackSessionAttributes gracedFeedbackSession =
                BackDoor.getFeedbackSession("SHomeUiT.CS2104", "Graced Feedback Session");
        // TODO: change to actual now once HTML time zone detection is fixed


Have you document it somewhere, e.g. issues in case we forget this later?

Just did - #8640.

tran-tien-dat · 2018-03-14T15:12:23Z

src/test/java/teammates/test/cases/datatransfer/FeedbackSessionAttributesTest.java

@@ -72,7 +72,8 @@ public void testBuilderCopy() {
                .withEndTime(TimeHelperExtension.getInstantHoursOffsetFromNow(5))
                .withSessionVisibleFromTime(TimeHelperExtension.getInstantHoursOffsetFromNow(1))
                .withResultsVisibleFromTime(TimeHelperExtension.getInstantHoursOffsetFromNow(6))
-                .withTimeZone(8).withGracePeriod(0).withFeedbackSessionType(FeedbackSessionType.PRIVATE)
+                .withTimeZone(TimeHelper.convertToZoneId(8)).withGracePeriod(0)


Just use something like ZoneId.of("+08:00")? convertToZoneId should be removed eventually.

tran-tien-dat · 2018-03-14T15:12:36Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java

@@ -680,7 +681,7 @@ private void testEnrollStudents() throws Exception {
                .withEndTime(TimeHelperExtension.getInstantHoursOffsetFromNow(5))
                .withSessionVisibleFromTime(TimeHelperExtension.getInstantHoursOffsetFromNow(1))
                .withResultsVisibleFromTime(TimeHelperExtension.getInstantHoursOffsetFromNow(6))
-                .withTimeZone(8)
+                .withTimeZone(TimeHelper.convertToZoneId(8))


Just use something like ZoneId.of("+08:00")? convertToZoneId should be removed eventually.

tran-tien-dat · 2018-03-14T15:12:47Z

src/test/java/teammates/test/pageobjects/InstructorFeedbackSessionsPage.java

-        addFeedbackSessionWithTimeZone(
-                feedbackSessionName, courseId, startTime, endTime, visibleTime, publishTime, instructions, gracePeriod, 8.0);
+        addFeedbackSessionWithTimeZone(feedbackSessionName, courseId, startTime, endTime, visibleTime, publishTime,
+                instructions, gracePeriod, TimeHelper.convertToZoneId(8));


Just use something like ZoneId.of("+08:00")? convertToZoneId should be removed eventually.

tran-tien-dat · 2018-03-14T15:21:53Z

Lol my comments point to error pages because of rebase.

whipermr5 · 2018-03-14T15:25:40Z

I think we should use something like ZoneId.of("+08:00") in instead of TimeHelper.convertToZoneId, as the later should be removed eventually once we finished migrating our UI and Database to ZoneId eventually.

@tran-tien-dat I did think of that, but decided that we'll want to relook all of the test time zone data when we eventually support geographical region ZoneIds - it should have a good mix of both fixed offset zones and regional zones that undergo DST. Hence, I used convertToZoneId as a way to mark all the test data where a fixed zone is hardcoded in test code.

darrenwee

👍 Just a query, otherwise all good from me.

darrenwee · 2018-03-14T15:19:55Z

src/main/java/teammates/common/datatransfer/attributes/FeedbackSessionAttributes.java

@@ -52,6 +53,8 @@
        respondingInstructorList = new HashSet<>();
        respondingStudentList = new HashSet<>();

+        timeZone = ZoneId.of("UTC");


Should this be extracted to Const as something like DEFAULT_TIME_ZONE? A similar fall back is used in CoursesDb.

Before the actual user interface is updated, tests will continue to use fixed offset time zones

tran-tien-dat · 2018-03-14T15:49:24Z

I did think of that, but decided that we'll want to relook all of the test time zone data when we eventually support geographical region ZoneIds - it should have a good mix of both fixed offset zones and regional zones that undergo DST. Hence, I used convertToZoneId as a way to mark all the test data where a fixed zone is hardcoded in test code.

I don't think that's necessary. I took a look and most of these fixed time zones come from the page ui tests. When we migrate the UI front-end to use geographical timezones, these tests will necessarily need to be updated to pick the geographical timezones. Hence, the current hardcoded double timezones are just temporary until we finish migration as well. Well that also means the changes I'm suggesting will also be modified eventually, so just keep it like what you did is fine.

tran-tien-dat · 2018-03-14T15:59:07Z

Hmm, but for the 3 occurences of hardcoded timezones in the logic tests StudentsLogicTest.java, FeedbackSessionAttributesTest.java, EmailGeneratorTest.java, time zones don't play much of any significant roles. They are just "check whether things have been copied correctly" kind of tests, so any time zones would be fine. You can just use geographical timezones here as well.

Furthermore, UTC time zones are currently hardcoded without the convertToZoneId anyway. So beside the reasons above, for consistency's sake, I revert back to my opinion that we should remove convertToZoneId from hard-coded timezones. Things that may get screwed up by DST time zones should have new tests for them.

EDIT: time zone does play a role in EmailGeneratorTest.java, but the test is explicitly testing for negative time zone, i.e. offset-based rather than geographical, so there's no point to mark this as potential place to change to using DST time zones. Instead, new tests should be written for DST geographical time zone here.

tran-tien-dat

LGTM

tran-tien-dat · 2018-03-14T16:16:52Z

@whipermr5 Travis is failing due to unused import 😛

whipermr5 added the s.Ongoing The PR is being worked on by the author(s) label Mar 14, 2018

whipermr5 force-pushed the 8638-migrate-fs-timeZone-field branch 3 times, most recently from 3ca7cf9 to 411ddae Compare March 14, 2018 11:55

whipermr5 added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Mar 14, 2018

whipermr5 requested review from tran-tien-dat and darrenwee March 14, 2018 12:31

whipermr5 force-pushed the 8638-migrate-fs-timeZone-field branch from 411ddae to dff2968 Compare March 14, 2018 15:15

tran-tien-dat requested changes Mar 14, 2018

View reviewed changes

darrenwee approved these changes Mar 14, 2018

View reviewed changes

whipermr5 added 7 commits March 14, 2018 23:47

Migrate FeedbackSessionAttributes and FeedbackSession

a29e166

Update FeedbackSessionsDb

9c855ef

Update EmailGenerator, EmailGeneratorTest

937297e

Add transition method TimeHelper.convertToOffset

8dab52f

Update UI layer

efa7101

Update tests

1bf41c2

Before the actual user interface is updated, tests will continue to use fixed offset time zones

Update JSON

5f6b959

whipermr5 force-pushed the 8638-migrate-fs-timeZone-field branch from dff2968 to 5f6b959 Compare March 14, 2018 15:48

Extract default time zone (UTC) into ZoneId constant

bfeecfb

whipermr5 requested a review from tran-tien-dat March 14, 2018 15:58

whipermr5 assigned tran-tien-dat and darrenwee Mar 14, 2018

tran-tien-dat approved these changes Mar 14, 2018

View reviewed changes

whipermr5 added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Mar 14, 2018

whipermr5 requested a review from damithc March 14, 2018 16:12

Avoid using TimeHelper.convertToZoneId in tests

089e3fa

whipermr5 force-pushed the 8638-migrate-fs-timeZone-field branch from 115079d to 089e3fa Compare March 14, 2018 16:22

whipermr5 added this to the V6.4.0 milestone Mar 14, 2018

whipermr5 added the e.1 label Mar 14, 2018

whipermr5 mentioned this pull request Mar 14, 2018

Migrate FeedbackSessionAttributes gracePeriod field to Duration #8641 #8642

Merged

damithc approved these changes Mar 15, 2018

View reviewed changes

damithc added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Mar 15, 2018

whipermr5 merged commit 6573c90 into TEAMMATES:master Mar 15, 2018

whipermr5 deleted the 8638-migrate-fs-timeZone-field branch March 15, 2018 01:30

whipermr5 added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Mar 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate feedback session timeZone field to ZoneId #8638 #8639

Migrate feedback session timeZone field to ZoneId #8638 #8639

whipermr5 commented Mar 14, 2018 •

edited

Loading

tran-tien-dat left a comment

tran-tien-dat Mar 14, 2018

whipermr5 Mar 14, 2018

tran-tien-dat Mar 14, 2018

tran-tien-dat Mar 14, 2018

tran-tien-dat Mar 14, 2018

tran-tien-dat Mar 14, 2018

tran-tien-dat Mar 14, 2018

tran-tien-dat Mar 14, 2018

whipermr5 Mar 14, 2018

tran-tien-dat Mar 14, 2018

tran-tien-dat Mar 14, 2018

tran-tien-dat Mar 14, 2018

tran-tien-dat commented Mar 14, 2018

whipermr5 commented Mar 14, 2018 •

edited

Loading

darrenwee left a comment

darrenwee Mar 14, 2018

tran-tien-dat commented Mar 14, 2018

tran-tien-dat commented Mar 14, 2018 •

edited

Loading

tran-tien-dat left a comment

tran-tien-dat commented Mar 14, 2018

Migrate feedback session timeZone field to ZoneId #8638 #8639

Migrate feedback session timeZone field to ZoneId #8638 #8639

Conversation

whipermr5 commented Mar 14, 2018 • edited Loading

tran-tien-dat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tran-tien-dat commented Mar 14, 2018

whipermr5 commented Mar 14, 2018 • edited Loading

darrenwee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tran-tien-dat commented Mar 14, 2018

tran-tien-dat commented Mar 14, 2018 • edited Loading

tran-tien-dat left a comment

Choose a reason for hiding this comment

tran-tien-dat commented Mar 14, 2018

whipermr5 commented Mar 14, 2018 •

edited

Loading

whipermr5 commented Mar 14, 2018 •

edited

Loading

tran-tien-dat commented Mar 14, 2018 •

edited

Loading