Migrate FeedbackSessionAttributes time fields from Date to Instant #8628 #8629

whipermr5 · 2018-03-12T06:57:42Z

Script used to migrate timestamps in JSON data:

import json
import os, io
from collections import OrderedDict
from datetime import datetime, timedelta

P_FORMAT = '%Y-%m-%d %I:%M %p UTC'

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 ['createdTime', 'startTime', 'endTime', 'sessionVisibleFromTime', 'resultsVisibleFromTime']:
                timestamp = datetime.strptime(fs[key], P_FORMAT)
                fs[key] = timestamp.isoformat() + 'Z'

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

darrenwee

Most of it looks great to me 👍 Not sure if you wanna add javadocs for the boolean methods in FeedbackSessionAttributes because I think some of their behavior is not super intuitive.

darrenwee · 2018-03-12T07:58:51Z

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

-
-        return now.after(startTime) && differenceBetweenDeadlineAndNow < hours;
+    public boolean isClosedAfter(long hours) {
+        return Instant.now().plus(Duration.ofHours(hours)).isAfter(endTime);
    }

    public boolean isClosingWithinTimeLimit(int hours) {


Change param type from int to long

darrenwee · 2018-03-12T08:34:06Z

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

@@ -354,7 +333,7 @@ public boolean isPublished() {
        } else if (publishTime.equals(Const.TIME_REPRESENTS_NOW)) {
            return true;
        } else {
-            return publishTime.before(now);
+            return now.isAfter(publishTime) || now.equals(publishTime);


Use early returns instead for these if/elses?

tran-tien-dat · 2018-03-12T10:27:46Z

src/test/java/teammates/test/cases/action/StudentFeedbackSubmissionEditSaveActionTest.java

@@ -1126,7 +1128,8 @@ protected void testAccessControl() throws Exception {

    private void testGracePeriodAccessControlForStudents() {
        FeedbackSessionAttributes fs = typicalBundle.feedbackSessions.get("gracePeriodSession");
-        fs.setEndTime(TimeHelper.convertLocalDateToUtc(TimeHelper.getDateOffsetToCurrentTime(0), fs.getTimeZone()));
+        fs.setEndTime(TimeHelper.convertLocalDateTimeToInstant(
+                TimeHelperExtension.now(), TimeHelper.convertToZoneId(fs.getTimeZone())));


This logic seems long winded, probably a result of the fact that these times used to be in local time instead of UTC. Just fs.setEndTime(Instant.now()) would be sufficient here. Personally, I find TimeHelperExtension.now very awkward, i.e. it shouldn't be there. Wherever it's used, I feel like it's because of the long-winded logic such as this.

You're right; just tried and realised it isn't necessary for this test. However it's still needed in StudentHomePageUiTest due to a problem with the test itself. In that test, the end time of Graced Feedback Session is set to x hours before the current time where x is the zone offset for the session, such that when displayed in the user's time zone, it is printed as the current time in UTC. This is necessary for the test to pass due to broken time zone detection in HtmlHelper, where it searches for the string UTC+xxxx to detect the time zone. However, that particular page has no such string, so the zone is detected as UTC, and the search and replace for datetime.now only matches the current time in UTC. Since this is a problem with the test, I will explicitly set it as such, and it can be fixed in a future PR. Will remove TimeHelperExtension#now.

tran-tien-dat · 2018-03-12T10:37:24Z

src/main/java/teammates/common/util/Const.java

+        TIME_REPRESENTS_NOW = TimeHelper.parseInstant("1970-02-14 12:00 AM +0000");
+        TIME_REPRESENTS_DEFAULT_TIMESTAMP = TimeHelper.parseInstant("2011-01-01 12:00 AM +0000");
+
+        TIME_REPRESENTS_DEFAULT_TIMESTAMP_DATE = TimeHelper.convertInstantToDate(TIME_REPRESENTS_DEFAULT_TIMESTAMP);


Mark @Deprecated?

The field is marked deprecated a few lines up, during declaration.

…f new method getInstantHoursOffsetFromNow

Deprecate TimeHelper.isSpecialTime(Date) in favour of new method isSpecialTime(Instant)

…ributes

Remove now unused method isTimeWithinPeriod

AdminInstructorAccountAddAction: remove "update feedback session time" replace as it was not working anyway (no such string "2013-04-01 11:59 PM UTC" in the template InstructorSampleData.json)

Add TimeHelperExtension.now() utility method

Update TimeHelperExtension methods: Remove getInstantMillisOffsetFromNow in favour of new method getInstantMinutesOffsetFromNow Remove now unused convertToOptionValueInTimeDropDown(Date)

…ursOffsetToCurrentTime

…IMESTAMP_DATE and deprecate

tran-tien-dat

LGTM 😺

darrenwee

🥇 LGTM

damithc

Just a question.

damithc · 2018-03-14T14:19:01Z

src/test/java/teammates/test/cases/action/InstructorFeedbackAddActionTest.java

+                + "<span class=\"bold\">From:</span> 2012-01-31T16:00:00Z"
+                + "<span class=\"bold\"> to</span> 2014-12-31T16:00:00Z<br>"
+                + "<span class=\"bold\">Session visible from:</span> 2011-12-31T16:00:00Z<br>"
+                + "<span class=\"bold\">Results visible from:</span> 1970-06-22T00:00:00Z<br>"


I remember a recent PR changed our date display format to a 'universally understood' format. Are we reversing that here?

No; this is just the admin logging system. It uses toString on the timestamps. The default toString implementation for Instant results in this format.

teammates/src/main/java/teammates/ui/controller/InstructorFeedbackAddAction.java

Lines 69 to 76 in 4d8b090

statusToAdmin =

"New Feedback Session (" + fs.getFeedbackSessionName() + ") for Course "

+ "[" + fs.getCourseId() + "] created. "

+ "From: " + fs.getStartTime()

+ " to " + fs.getEndTime() + " "

+ "Session visible from: " + fs.getSessionVisibleFromTime() + " "

+ "Results visible from: " + fs.getResultsVisibleFromTime() + " "

+ "Instructions: " + fs.getInstructions();

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

whipermr5 force-pushed the 8628-migrate-fs-time-fields-instant branch from 2c0d62e to 1b103eb Compare March 12, 2018 07:18

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 12, 2018

whipermr5 requested review from tran-tien-dat and darrenwee March 12, 2018 07:49

darrenwee reviewed Mar 12, 2018

View reviewed changes

tran-tien-dat reviewed Mar 12, 2018

View reviewed changes

whipermr5 requested review from tran-tien-dat and darrenwee March 14, 2018 05:57

whipermr5 force-pushed the 8628-migrate-fs-time-fields-instant branch from e9a5a69 to 5a8931e Compare March 14, 2018 06:10

whipermr5 added 16 commits March 14, 2018 14:20

Remove TImeHelper.now() in favour of Instant.now()

9ea2aca

Deprecate TimeHelperExtension.getHoursOffsetToCurrentTime in favour o…

18a87d1

…f new method getInstantHoursOffsetFromNow

Add Instant versions of special time representations

4f27e03

Deprecate TimeHelper.isSpecialTime(Date) in favour of new method isSpecialTime(Instant)

Migrate *Time fields from Date to Instant in feedback session and att…

5a463c5

…ributes

Update FieldValidator and FieldValidatorTest

5971308

Update FeedbackSessionsDb

f5fd307

Remove now unused method isTimeWithinPeriod

Update logic layer

a98f164

Update UI layer

37be125

AdminInstructorAccountAddAction: remove "update feedback session time" replace as it was not working anyway (no such string "2013-04-01 11:59 PM UTC" in the template InstructorSampleData.json)

Update tests: page data

2f2cb8d

Update tests: attributes

81034fd

Update FeedbackSessionsDbTest

36c6495

Update tests: logic

74dfbda

Update tests: automated

f6ab840

Update tests: action

3b4792c

Add TimeHelperExtension.now() utility method

Update tests: UI (part 1)

148ce20

Update tests: UI (part 2)

04b8b87

Update TimeHelperExtension methods: Remove getInstantMillisOffsetFromNow in favour of new method getInstantMinutesOffsetFromNow Remove now unused convertToOptionValueInTimeDropDown(Date)

whipermr5 added 8 commits March 14, 2018 14:20

Update JSON timestamps

034bcc8

Update hardcoded timestamps

86dcf6c

Remove now unused methods combineDateTime, isSpecialTime(Date), getHo…

5d2b958

…ursOffsetToCurrentTime

Rename combineDateTimeNew -> combineDateTime

3a7ba15

Rename TIME_REPRESENTS_DEFAULT_TIMESTAMP -> TIME_REPRESENTS_DEFAULT_T…

771a3fb

…IMESTAMP_DATE and deprecate

Rename special time representation constants back to original names

8cc9180

Remove TimeHelperExtension.now()

d6b8eb9

Fix style issues for isClosingWithinTimeLimit and isPublished

3c4eb3a

whipermr5 force-pushed the 8628-migrate-fs-time-fields-instant branch from 5a8931e to 3c4eb3a Compare March 14, 2018 06:21

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

View reviewed changes

darrenwee approved these changes Mar 14, 2018

View reviewed changes

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

whipermr5 requested a review from damithc March 14, 2018 07:26

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 mentioned this pull request Mar 14, 2018

Migrate *Attributes time fields from Date to Instant #8636 #8637

Merged

damithc approved these changes Mar 14, 2018

View reviewed changes

whipermr5 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 14, 2018

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

whipermr5 added the e.2 label Mar 14, 2018

whipermr5 merged commit 30f0012 into TEAMMATES:master Mar 14, 2018

whipermr5 deleted the 8628-migrate-fs-time-fields-instant branch March 14, 2018 15:00

whipermr5 mentioned this pull request Mar 14, 2018

HtmlHelper: Use better way to replace now timestamps with datetime.now #8640

Closed

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

	statusToAdmin =
	"New Feedback Session <span class=\"bold\">(" + fs.getFeedbackSessionName() + ")</span> for Course "
	+ "<span class=\"bold\">[" + fs.getCourseId() + "]</span> created.<br>"
	+ "<span class=\"bold\">From:</span> " + fs.getStartTime()
	+ "<span class=\"bold\"> to</span> " + fs.getEndTime() + "<br>"
	+ "<span class=\"bold\">Session visible from:</span> " + fs.getSessionVisibleFromTime() + "<br>"
	+ "<span class=\"bold\">Results visible from:</span> " + fs.getResultsVisibleFromTime() + "<br><br>"
	+ "<span class=\"bold\">Instructions:</span> " + fs.getInstructions();

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate FeedbackSessionAttributes time fields from Date to Instant #8628 #8629

Migrate FeedbackSessionAttributes time fields from Date to Instant #8628 #8629

whipermr5 commented Mar 12, 2018 •

edited

Loading

darrenwee left a comment

darrenwee Mar 12, 2018

darrenwee Mar 12, 2018

tran-tien-dat Mar 12, 2018

whipermr5 Mar 14, 2018

tran-tien-dat Mar 12, 2018 •

edited

Loading

whipermr5 Mar 12, 2018

tran-tien-dat left a comment

darrenwee left a comment

damithc left a comment

damithc Mar 14, 2018

whipermr5 Mar 14, 2018 •

edited

Loading

Migrate FeedbackSessionAttributes time fields from Date to Instant #8628 #8629

Migrate FeedbackSessionAttributes time fields from Date to Instant #8628 #8629

Conversation

whipermr5 commented Mar 12, 2018 • edited Loading

darrenwee left a comment

Choose a reason for hiding this comment

darrenwee Mar 12, 2018

Choose a reason for hiding this comment

darrenwee Mar 12, 2018

Choose a reason for hiding this comment

tran-tien-dat Mar 12, 2018

Choose a reason for hiding this comment

whipermr5 Mar 14, 2018

Choose a reason for hiding this comment

tran-tien-dat Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

whipermr5 Mar 12, 2018

Choose a reason for hiding this comment

tran-tien-dat left a comment

Choose a reason for hiding this comment

darrenwee left a comment

Choose a reason for hiding this comment

damithc left a comment

Choose a reason for hiding this comment

damithc Mar 14, 2018

Choose a reason for hiding this comment

whipermr5 Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

whipermr5 commented Mar 12, 2018 •

edited

Loading

tran-tien-dat Mar 12, 2018 •

edited

Loading

whipermr5 Mar 14, 2018 •

edited

Loading