Display comment time in the session's time zone #4488 #7616

Mynk96 · 2017-06-27T07:35:38Z

Mynk96 · 2017-06-27T09:11:43Z

Ready For Review!

leeyimin

Some issues to be solved before I can give a more detailed review!

leeyimin · 2017-06-29T06:07:53Z

src/main/java/teammates/storage/search/FeedbackResponseCommentSearchDocument.java

@@ -300,6 +300,7 @@ public static FeedbackResponseCommentSearchResultBundle fromResults(
                frcDb.deleteDocument(comment);
                continue;
            }
+            bundle.sessionTimeZone = session.getTimeZone();


The time zone of the bundle would be the time zone of the session of the last document in the filtered results. If there are multiple sessions in the filtered result, the time zone of certain comment rows may be wrong.

leeyimin · 2017-06-29T06:16:15Z

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

+        mapDoubleToId("12.0", "Pacific/Auckland");
+        mapDoubleToId("12.75", "Pacific/Chatham");
+        mapDoubleToId("13.0", "Pacific/Tongatapu");
+        mapDoubleToId("14.0", "Pacific/Kiritimati");


This may not be the best way to translate the double to a TimeZone object. I tested with some of the time zones which corresponded to the US cities in the list and DST was taken in account (eg. UTC -0800 shows up as -0700), which may be unexpected for users.

https://docs.oracle.com/javase/7/docs/api/java/util/TimeZone.html
Using the custom time zone ID may be more appropriate here, and it can be generated by code as well.

…ime-zone

…ttps://github.com/Mynk96/teammates into 4488-Display-comment-time-in-the-session-time-zone

Mynk96 · 2017-06-29T22:34:11Z

@leeyimin Can you please have a look at the build error!

leeyimin · 2017-06-30T03:53:42Z

src/main/java/teammates/common/datatransfer/FeedbackResponseCommentSearchResultBundle.java

+    public Map<String, String> responseGiverTable = new HashMap<String, String>();
+    public Map<String, String> responseRecipientTable = new HashMap<String, String>();
+    public Set<String> instructorEmails = new HashSet<String>();
+    public Map<String, String> instructorEmailNameTable = new HashMap<String, String>();


Are all these changes necessary?

leeyimin · 2017-06-30T03:55:35Z

src/main/java/teammates/storage/search/FeedbackResponseCommentSearchDocument.java

@@ -300,9 +300,11 @@ public static FeedbackResponseCommentSearchResultBundle fromResults(
                frcDb.deleteDocument(comment);
                continue;
            }
+


Revert the added new line.

leeyimin · 2017-06-30T03:57:49Z

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

+        if (minutes != 0) {
+            offset = Integer.toString(hours) + ":" + Integer.toString(minutes);
+        }
+        String timeZone = TimeZone.getTimeZone("GMT" + sign + offset).getID();


Should extract this part as a separate method.

leeyimin · 2017-06-30T04:15:43Z

src/test/resources/pages/instructorFeedbackResultsAddComment.html

@@ -358,7 +358,7 @@
                              <li class="list-group-item list-group-item-warning" id="responseCommentRow-0-1-0-1-1">
                                <div id="commentBar-0-1-0-1-1">
                                  <span class="text-muted">
-                                    From: Teammates Test [${datetime.now}]
+                                    From: Teammates Test [Fri, 30 Jun 2017, 05:24 AM +0800]


You need to update the replaceUnpredictableValuesWithPlaceholders method in HtmlHelper. As the time zone of the comment time has changed, the HtmlHelper is unable to replace occurrences of current time correctly. One solution I can think of now is to loop through all possible time zones and replace all possible current time representations with the placeholder. Would be good if you can find a better solution 😂

Ideal case: detect the time zone and replace with ${datetime.now.utc+8}

Mynk96 · 2017-07-04T07:09:03Z

@leeyimin Still working please don't review it now!

leeyimin · 2017-07-13T04:44:14Z

src/test/java/teammates/test/driver/HtmlHelper.java

@@ -385,6 +389,8 @@ private static String suppressVariationsInInjectedValues(String content) {
    private static String replaceUnpredictableValuesWithPlaceholders(String content) {
        Date now = new Date();
        SimpleDateFormat sdf = new SimpleDateFormat("EEE, dd MMM yyyy, ");
+        //A page can have only one time zone as we cannot have session with two different time zones.


A page can have two time zones since instructor comments can show comments from different courses and sessions. You may want to correct your comment.

You should only mention that the time zone of expected dateTimeNow is taken from the content.

leeyimin · 2017-07-13T04:49:52Z

src/test/java/teammates/test/driver/HtmlHelper.java

+        Matcher matcher = pattern.matcher(content);
+        //set default timezone offset.
+        String timeZoneOffset = "+0000";
+        while (matcher.find()) {


Why is while used here?

leeyimin · 2017-07-13T04:55:13Z

src/test/java/teammates/test/driver/HtmlHelper.java

@@ -36,9 +39,10 @@
    private static final String REGEX_BLOB_KEY = "(encoded_gs_key:)?[a-zA-Z0-9-_]{10,}";
    private static final String REGEX_QUESTION_ID = "[a-zA-Z0-9-_]{40,}";
    private static final String REGEX_COMMENT_ID = "[0-9]{16}";
-    private static final String REGEX_DISPLAY_TIME = "(0[0-9]|1[0-2]):[0-5][0-9] [AP]M( UTC)?";
+    private static final String REGEX_DISPLAY_TIME = "(0[0-9]|1[0-2]):[0-5][0-9] [AP]M?|NOON";


The regex should be improved, it matches "12:00 A", "NOON" but not "12:00 NOON".

Mynk96 · 2017-07-18T03:49:29Z

src/test/java/teammates/test/driver/HtmlHelper.java

        Pattern pattern = Pattern.compile(REGEX_TIMEZONE_OFFSET);
        Matcher matcher = pattern.matcher(content);
-        //set default timezone offset.
+        //set default time zone offset.
        String timeZoneOffset = "+0000";
        while (matcher.find()) {


As the content will have comments from single time zone, I am finding the last word with Pattern 'UTC+XXXX` and extracting time zone offset from it.

Why is the last word with UTC+XXXX used, not the first one?

because using pattern +xxxx it also matches string such as responseCommentRow-1234567812345678 in the content file.

I believe you can simply use the first word of UTC+XXXX. Is there any reason you must use the last one?

Yes, we can also use the first one.

leeyimin · 2017-07-18T04:00:47Z

src/test/java/teammates/test/driver/HtmlHelper.java

@@ -385,6 +389,8 @@ private static String suppressVariationsInInjectedValues(String content) {
    private static String replaceUnpredictableValuesWithPlaceholders(String content) {
        Date now = new Date();
        SimpleDateFormat sdf = new SimpleDateFormat("EEE, dd MMM yyyy, ");
+        //Get session's time zone from content.


You should mention in the comment that this assumes that the page only shows one timezone as it is possible for a page to show multiple time zones (eg. InstructorSearchPage) and this method may not work for such a page.

Add a space after // and use lower case for get.

leeyimin · 2017-07-18T04:02:50Z

src/test/java/teammates/test/driver/HtmlHelper.java

+    }
+
+    private static TimeZone getTimeZone(String content) {
+        //Searches for last String of pattern "UTC+0800" in the content.


This comment does not seem relevant. You should not be searching for "UTC+8000" in particular.

leeyimin · 2017-07-18T04:06:00Z

src/test/java/teammates/test/driver/HtmlHelper.java

@@ -36,9 +39,10 @@
    private static final String REGEX_BLOB_KEY = "(encoded_gs_key:)?[a-zA-Z0-9-_]{10,}";
    private static final String REGEX_QUESTION_ID = "[a-zA-Z0-9-_]{40,}";
    private static final String REGEX_COMMENT_ID = "[0-9]{16}";
-    private static final String REGEX_DISPLAY_TIME = "(0[0-9]|1[0-2]):[0-5][0-9] [AP]M( UTC)?";
+    private static final String REGEX_DISPLAY_TIME = "(0[0-9]|1[0-2]):[0-5][0-9] ([AP]M?|NOON)";


I believe the ? after M is unnecessary?

leeyimin · 2017-07-18T04:07:49Z

src/test/java/teammates/test/driver/HtmlHelper.java

+    }
+
+    private static String removeUtc(String timeZoneOffset) {
+        return timeZoneOffset.substring(0, 2);


The method can be improved, it should still work for time zones which are not integer offsets (eg. +0330).

This method is just converting string from UTC+xxxx to xxxx.

I believe you are taking the first 2 characters of timeZoneOffset, which means it does not even work for time zones other than +0000.

No, I am searching for the last string with a pattern as UTC+xxxx and then extracting +xxxx from it.For example, timeZoneOffset is UTC-0330 then this will return -0330.

By matching group 1, and not group 0, you are already extracting +xxxx. Please read up on methods you use as well, String.substring(int beginIndex, int endIndex) returns the first 2 characters. It does not remove the first three characters.

Apologies. I have improved the method.

Mynk96 · 2017-07-18T05:58:25Z

@leeyimin Please Review!

leeyimin · 2017-07-18T05:56:43Z

src/test/java/teammates/test/driver/HtmlHelper.java

+    }
+
+    private static TimeZone getTimeZone(String content) {
+        // searches for last String of pattern "UTC+xxxx" in the content.


Update your comment.

leeyimin · 2017-07-18T05:57:54Z

src/test/java/teammates/test/driver/HtmlHelper.java

@@ -385,6 +389,8 @@ private static String suppressVariationsInInjectedValues(String content) {
    private static String replaceUnpredictableValuesWithPlaceholders(String content) {
        Date now = new Date();
        SimpleDateFormat sdf = new SimpleDateFormat("EEE, dd MMM yyyy, ");
+        // get session's time zone from content.


Mention that this assumes only the page only shows time in one time zones, there are pages which can show multiple time zones (eg. InstructorSearchPage) and this method may not work on such pages.

leeyimin · 2017-07-18T05:58:11Z

src/test/java/teammates/test/driver/HtmlHelper.java

+        // searches for last String of pattern "UTC+xxxx" in the content.
+        Pattern pattern = Pattern.compile(REGEX_TIMEZONE_OFFSET);
+        Matcher matcher = pattern.matcher(content);
+        //set default time zone offset.


space after //.

leeyimin · 2017-07-18T06:30:35Z

src/test/java/teammates/test/driver/HtmlHelper.java

@@ -385,6 +389,9 @@ private static String suppressVariationsInInjectedValues(String content) {
    private static String replaceUnpredictableValuesWithPlaceholders(String content) {
        Date now = new Date();
        SimpleDateFormat sdf = new SimpleDateFormat("EEE, dd MMM yyyy, ");
+        // get session's time zone from content (content here includes pages only with one time zone).


content here includes pages only with one time zone is not accurate. Remove this part?

Should I replace it with something or // get session's time zone from content. is fine?

Yes, that would be fine.

leeyimin

LGTM!

HirdayGupta · 2017-07-18T09:27:33Z

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

+    public static TimeZone getTimeZoneFromDoubleOffset(double sessionTimeZone) {
+        int hours = (int) sessionTimeZone;
+        int minutes = (int) ((Math.abs(sessionTimeZone) - Math.floor(Math.abs(sessionTimeZone))) * 60);
+        return TimeZone.getTimeZone(String.format("GMT%+03d:%02d", hours, minutes));


Could you maybe change from GMT to UTC? As UTC is the time standard used in TEAMMATES and it is unaffected by Daylight savings.

The Java TimeZone object only accepts GMT. Note that this method returns a TimeZone, not a string.

Ohh I see. Its okay then I guess. :)

…ime-zone

@Mynk96

…ffset Using the method added by @Mynk96 in TEAMMATES#7616

Mynk96 added 4 commits June 27, 2017 07:31

Displayed time according to session timezone and updated tests

1cb7d26

Resolved HtmlHelperTest.java

c464fb0

Updated AdminHomePageUiTest

481adff

fixed HtmlHelperTest

c5f9c07

leeyimin added the s.ToReview The PR is waiting for review(s) label Jun 27, 2017

whipermr5 assigned leeyimin Jun 27, 2017

whipermr5 requested a review from leeyimin June 27, 2017 11:01

leeyimin requested changes Jun 29, 2017

View reviewed changes

leeyimin added s.Ongoing The PR is being worked on by the author(s) and removed s.ToReview The PR is waiting for review(s) labels Jun 29, 2017

Mynk96 added 8 commits June 29, 2017 19:32

Used custom time zone ID

a11c530

Corrected time zones for comment rows for search results

90569e3

updated offset format in TimeHelper

f8c31f4

Merge branch 'master' into 4488-Display-comment-time-in-the-session-t…

9ca8282

…ime-zone

resolved PMD error

756ef9a

Merge branch '4488-Display-comment-time-in-the-session-time-zone' of h…

f058809

…ttps://github.com/Mynk96/teammates into 4488-Display-comment-time-in-the-session-time-zone

updated tests

b971f3b

reverted changes made using godmode

89b98b6

leeyimin requested changes Jun 30, 2017

View reviewed changes

Mynk96 added 3 commits July 4, 2017 06:11

resolved merge conflicts

4f921be

Updated tests using godMode

47f1eb2

Requested changes

7fb1a74

leeyimin 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 Jul 4, 2017

Mynk96 added 2 commits July 4, 2017 06:40

Cosmetic changes

548e2b1

Updated HtmlHelperTest

73e4594

Setted proper offset time sign

68e9dde

Mynk96 added 2 commits July 13, 2017 00:14

updated TimeHelperTest.java

affead3

Added UTC to time format and tests

1dea961

leeyimin requested changes Jul 13, 2017

View reviewed changes

Mynk96 added 3 commits July 13, 2017 18:50

refactored code

26faa9b

merge conflicts

be8e028

improved regex and added comments

b9ddf2c

Mynk96 commented Jul 18, 2017

View reviewed changes

leeyimin requested changes Jul 18, 2017

View reviewed changes

Mynk96 added 3 commits July 18, 2017 09:52

refactored comments

a96dfd6

improved logic for getTimeZone

6ae0d45

used first matched time zone string

b1d6424

leeyimin requested changes Jul 18, 2017

View reviewed changes

updated comments

e8b9694

leeyimin reviewed Jul 18, 2017

View reviewed changes

removed extra comment

3fae357

leeyimin approved these changes Jul 18, 2017

View reviewed changes

leeyimin added s.FinalReview The PR is ready for final review and removed s.Ongoing The PR is being worked on by the author(s) labels Jul 18, 2017

leeyimin requested a review from damithc July 18, 2017 07:05

damithc approved these changes Jul 18, 2017

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 Jul 18, 2017

whipermr5 added this to the V5.110 milestone Jul 18, 2017

leeyimin added the e.4 label Jul 18, 2017

HirdayGupta reviewed Jul 18, 2017

View reviewed changes

Merge branch 'master' into 4488-Display-comment-time-in-the-session-t…

f2794e2

…ime-zone

whipermr5 merged commit 4257d89 into TEAMMATES:master Jul 18, 2017

HirdayGupta added a commit to HirdayGupta/teammates that referenced this pull request Jul 18, 2017

Use new helper method instead of previous method to format timezone o…

22c7464

…ffset Using the method added by @Mynk96 in TEAMMATES#7616

whipermr5 mentioned this pull request Mar 14, 2018

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

Closed

Display comment time in the session's time zone #4488 #7616

Display comment time in the session's time zone #4488 #7616

Conversation

Mynk96 commented Jun 27, 2017

Mynk96 commented Jun 27, 2017

leeyimin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mynk96 commented Jun 29, 2017 • edited Loading

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

Mynk96 commented Jul 4, 2017 • edited Loading

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

leeyimin Jul 18, 2017 • edited Loading

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

leeyimin Jul 18, 2017 • edited Loading

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

Mynk96 commented Jul 18, 2017

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

Mynk96 Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leeyimin left a comment

Choose a reason for hiding this comment

HirdayGupta Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

whipermr5 Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mynk96 commented Jun 29, 2017 •

edited

Loading

Mynk96 commented Jul 4, 2017 •

edited

Loading

leeyimin Jul 18, 2017 •

edited

Loading

leeyimin Jul 18, 2017 •

edited

Loading

Mynk96 Jul 18, 2017 •

edited

Loading

HirdayGupta Jul 18, 2017 •

edited

Loading

whipermr5 Jul 18, 2017 •

edited

Loading