Add support for daylight saving time #84 #8687

whipermr5 · 2018-03-20T10:23:51Z

Fixes #84

Outline of Solution

Accept only geographical time zones from the session add/edit actions
New geographical time zone input UI
- Sessions form
- Add course panel
- Edit course panel
Update UI to show time zone names instead of UTC offsets
- Emails
- Submission: session details panel
- Results: session details panel (previously did not even show time zone)
- Comment timestamps
- Instructor home page not updated yet as this would make the sort by date even more broken than it already is; will fix after Instructor: home page: fix sort by date #7919
Fixed-offset time zones are still valid from the logic layer downwards so existing sessions continue to work as per normal
Show a modal warning when editing existing sessions, prompting users to re-save the session with the new geographical time zone (auto-detected)
- Close-up
- Animation
Validate local date times input by the user when adding/editing feedback sessions, checking for ambiguity
- Resolve ambiguous date times using java.time default rules
- Show a warning for each detected ambiguous date time

Other Changes

A new exception type InvalidPostParametersException was added
- NullPostParameterException extends InvalidPostParametersException
Feedback session add and edit actions now strictly validate POST parameters for cleaner code, throwing InvalidPostParametersException (and hence redirecting to the instructor home page) if any mischief (custom POST request crafting/ DOM manipulation) is detected (see 32eea36)

wkurniawan07 · 2018-03-20T15:51:32Z

@whipermr5 may not be directly related, but do you still plan to keep both the timezone fields of the feedback session or course? Surely one of them has got to go.

whipermr5 · 2018-03-22T06:40:38Z

@wkurniawan07 The one in the feedback session will eventually go, but not within this PR.

whipermr5 · 2018-03-22T07:15:28Z

@tran-tien-dat @darrenwee Y'all can do an early review first; I'm left with UI tests and new tooltips/warnings to make it more obvious to existing instructors that we've started supporting DST.

tran-tien-dat · 2018-03-22T16:45:14Z

src/main/webapp/WEB-INF/tags/instructor/feedbacks/feedbackSessionsForm.tag

                </select>
+                <input type="button" class="btn btn-primary" id="auto-detect-time-zone" value="Auto-Detect Time Zone">


If I remember correctly, for course there are some texts to explain to the user that the timezone is automatically detected. I think it's good to have here too

tran-tien-dat · 2018-03-22T16:51:56Z

src/main/webapp/dev/js/common/timezone.js

-            $selectElement.val(timeZone);
+        if (moment.tz.names().indexOf(timeZone) === -1) {
+            const o = document.createElement('option');
+            o.text = `${timeZone} (please select one of the above options)`;


Hmm, what's the purpose of this?

For existing sessions that use fixed offset ZoneIds. The fixed offset ID will be displayed here as the final option, and the user will have to select a proper regional time zone to be able to save the feedback session (fixed offset ZoneIds are disallowed at the action level by the backend). This will be more obvious once I've added a warning to the user to choose another option.

Yeah, you can add a comment to remind that it's for backward compatibility.

tran-tien-dat · 2018-03-22T17:11:08Z

src/main/webapp/dev/js/common/instructorFeedbacks.js

@@ -126,11 +130,30 @@ function formatSessionVisibilityGroup() {
    });
 }

+function initializeTimeZoneOptions() {


Should we use this in the course time zone selection as well?

Yes, though the course edit page will have to be modified. I'll try and if the changes are not too extensive, I'll include them in this PR.

tran-tien-dat · 2018-03-22T17:44:49Z

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

-            } catch (DateTimeException e) {
-                log.warning("Failed to parse time zone parameter: " + paramTimeZone);
-            }
+        Assumption.assertPostParamNotNull(Const.ParamsNames.FEEDBACK_SESSION_TIMEZONE, paramTimeZone);


Hmm. From the comments at the beginning of this function: this field can be null when editting feedback session. Yet another problem: when we create a FeedbackSessionAttributes it will always have a non-null time zone. So if the user doesn't want to change the timezone, the backend actually resets the timezone to default.

I think FeedbackSessionAttributes shouldn't be used to transfer data for the edit action.

Yes; ideally the flow should be to get the existing session attributes, then update the changed fields. However I feel this is outside the scope of this PR.

tran-tien-dat · 2018-03-22T17:45:12Z

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

+        try {
+            attributes.setTimeZone(ZoneId.of(paramTimeZone));
+        } catch (DateTimeException e) {
+            // Leave the attributes time zone field at its default valid value (i.e. UTC)


Problematic, as my previous comment.

Well, the only reason why the POST param would be null or invalid is mischief from the user, so I don't view this refactoring as important.

I think you missed my point in the previous comment. null POST param is valid when editting feedbacksession. It means do not change this field. But here we are updating the field to UTC.

I agree with you that from the practical point of view, if the front end always send data with all these fields, then none of these exceptions will happen when the user is using the website. However, from the developer's point of view, this class is problematic because the code is contradicting with the comments (specifically the comments at the start of extractFeedbackSessionData). This will lead to confusion for future developer who reads the code, as well as some inconsistencies in the code, e.g. why does it only require time zone to not be null? How about start time, end time, etc.? So the comment only applies to the times but not the time zone?

One thing that definitely needs to be fixed is that the comments have to match what the code is doing. So I can see 2 choices here:

Backend assumes frontend will send every fields. Then we can consistently require not-null for all fields and remove many of these isCreatingNewSession clauses that make things more complicated.

Keep the code as it is and update in the comments that timezone must always be supplied, but other fields can be null, in which case the field will not be updated.

tran-tien-dat · 2018-03-22T18:04:22Z

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

+        if (!timeZoneErrorMessage.isEmpty()) {
+            // Collect other errors before throwing an exception
+            List<String> errors = new ArrayList<>();
+            // When editing an existing session, this will fail as some fields might be validly set to null


Or timezone field might be validly set to null and fails the validation as well (i.e. same problems as in extractFeedbackSessionData)

darrenwee

Looks 👍

There is no test for a situation with an overlap start time. I don't think it's super necessary, so your discretion.
There is no test for a situation where the grace period causes responses to be accepted up to a time which is a GAP or OVERLAP
Sometimes timezone rules get changed by governments. It would also lead to either a GAP or OVERLAP situation too, but isn't caused by daylight savings. Should we test this too? The most extreme example I could find is Samoa which switched her standard time from UTC-11:00 to UTC+13:00 in 2011.

darrenwee · 2018-03-23T06:08:37Z

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

+
+        params[25] = "Europe/Andorra";
+        params[5] = "Sun, 25 Mar, 2012";
+        params[7] = "2"; // Sun 25 Mar, 2012, 02:00 AM falls in the gap period when clocks sprang forward in Andorra


Add context for self-contained doc:

After Sunday, 25 March, 2012 01:59:59 AM, clocks were moved forward to become Sunday, 25 March, 2012 03:00:00 AM

darrenwee · 2018-03-23T06:08:57Z

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

+        params[5] = "Sun, 25 Mar, 2012";
+        params[7] = "2"; // Sun 25 Mar, 2012, 02:00 AM falls in the gap period when clocks sprang forward in Andorra
+        params[9] = "Sun, 28 Oct, 2012";
+        params[11] = "2"; // Sun 28 Oct, 2012, 02:00 AM falls in the overlap period when clocks fell back in Andorra


Same as above:

After Sunday, 28 October, 2012 02:59:59 AM: clocks were moved backward to become Sunday, 28 October, 2012 02:00:00 AM

darrenwee · 2018-03-23T06:10:06Z

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

+                         instructor1ofCourse1.courseId, "Course with DST time zone");
+        params[25] = "Asia/Jerusalem";
+        params[9] = "Fri, 28 Mar, 2014";
+        params[11] = "2"; // Fri 28 Mar, 2014, 02:00 AM falls in the gap period when clocks sprang forward in Israel


Add context for self-contained comment:

After Friday, 28 March, 2014 01:59:59 AM: clocks were moved forward to become Friday, 28 March, 2014 03:00:00 AM

darrenwee · 2018-03-23T06:10:56Z

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

+                instructor1ofCourse1.courseId, longFsName, 1);
+        params[25] = "Asia/Jerusalem";
+        params[21] = "Sun, 25 Oct, 2015";
+        params[23] = "1"; // Sun 25 Oct, 2015, 01:00 AM falls in the overlap period when clocks fell back in Israel


Add context for self-contained doc:

After Sunday, 25 October, 2015 01:59:59 AM: clocks were moved backward to become Sunday, 25 October, 2015 01:00:00 AM

tran-tien-dat · 2018-03-23T09:14:33Z

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

-        // without accounts need to receive the email to be able to respond
-        feedbackSession.setOpeningEmailEnabled(true);
+        // This is only for validation to pass; it will be overridden with its existing value at the logic layer
+        String dummyCreatorEmail = "dummy@example.com";


I think it's better to not use a dummy value. Can extract the instructor from line 23 and use instructor.email

The instructor may not be the creator (e.g. a co-owner).

I see. Nvm then.

whipermr5 · 2018-03-23T09:14:45Z

There is no test for a situation where the grace period causes responses to be accepted up to a time which is a GAP or OVERLAP

@darrenwee Can explain this more? I don't think it's necessary as end time is an instant and grace period is a duration that is compared with now and the instant to determine whether the session is closed?

…d edit panels Use input group for neater auto-detect button placement Remove redundant "Select a timezone..." option

Include select element in HTML tests but remove options Useful to check tooltip, selected time zone and disabled attributes

Superclass of NullPostParameterException

Remove isCreatingNewSession flag Throw burden of sanitising name and supplying course ID and creator email to users of the method Assert all relevant post parameters not null using getNonNullRequestParamValue Throw InvalidPostParametersException if any mischief is detected Accept responsibility for setting opening email enabled flag to true Remove unnecessary setting of sent open and published email flags as these are already false by default Always set created time to now since this will be overridden for updating of existing sessions anyway Also remove isCreatingNewSession flag from validateTimeData Remove an unused parameter name constant that was never set in the frontend

…ixed offset modal warning

Use short zone name instead of UTC offset

Temporary workaround to allow sorting to continue as per normal

sukanta-27 · 2018-04-02T08:18:42Z

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

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


@whipermr5 I was resolving the conflicts in #8447, Wanted to know how you chose these dates. Specially session Visible from and results visible from dates. I mean does these particular dates signify something?

They are defined in Const.java.

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

whipermr5 force-pushed the 84-dst branch 9 times, most recently from 0c5310e to 93e131b Compare March 22, 2018 04:33

darrenwee requested review from tran-tien-dat and darrenwee March 22, 2018 07:18

darrenwee self-assigned this Mar 22, 2018

whipermr5 force-pushed the 84-dst branch from 93e131b to 060a165 Compare March 22, 2018 15:41

tran-tien-dat reviewed Mar 22, 2018

View reviewed changes

whipermr5 assigned tran-tien-dat Mar 22, 2018

tran-tien-dat reviewed Mar 22, 2018

View reviewed changes

whipermr5 force-pushed the 84-dst branch from 060a165 to 3e7b629 Compare March 23, 2018 04:46

darrenwee reviewed Mar 23, 2018

View reviewed changes

whipermr5 force-pushed the 84-dst branch 2 times, most recently from 0b78240 to 30b531f Compare March 23, 2018 08:41

whipermr5 requested a review from tran-tien-dat March 23, 2018 08:46

tran-tien-dat reviewed Mar 23, 2018

View reviewed changes

whipermr5 added 18 commits April 1, 2018 17:21

Extract timeZoneInput.tag and use for sessions form and course add an…

32eb18f

…d edit panels Use input group for neater auto-detect button placement Remove redundant "Select a timezone..." option

Adjust tooltip for time zone input in sessions form

28cd1a0

Show modal alert when editing a session with a fixed offset time zone

784b267

Update instructor help UI examples

08fae9c

Update test HTML files

cf1d3e0

Include select element in HTML tests but remove options Useful to check tooltip, selected time zone and disabled attributes

Create new exception type InvalidPostParametersException

754cfdb

Superclass of NullPostParameterException

Update action tests

7c445f8

InstructorFeedbackEditPageUiTest: test for ambiguous date times and f…

2d447fe

…ixed offset modal warning

InstructorFeedbackSessionsPageUiTest: test for ambiguous date time

05ae71f

Fix remaining test cases using fixed offset time zones

f065ef0

Change formatDateTimeForSessions format

92ffe67

Use short zone name instead of UTC offset

Use old format for instructor home page

4974414

Temporary workaround to allow sorting to continue as per normal

Update hardcoded test cases

1a8310e

Update test HTML files

26fabfb

Show time zone in results session panel

2055951

Update test HTML files

e4fb929

Adjust tooltip

e49d785

whipermr5 force-pushed the 84-dst branch from b833643 to e49d785 Compare April 1, 2018 10:30

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 Apr 1, 2018

whipermr5 merged commit 76ab9a0 into TEAMMATES:master Apr 1, 2018

whipermr5 mentioned this pull request Apr 1, 2018

Fix indentation for test resource html pages to avoid irrelevant changes #8744

Closed

whipermr5 deleted the 84-dst branch April 1, 2018 16:40

sukanta-27 reviewed Apr 2, 2018

View reviewed changes

whipermr5 added the c.Feature User-facing feature; can be new feature or enhancement to existing feature label Apr 14, 2018

whipermr5 mentioned this pull request Apr 17, 2018

Instructor: edit session: visible date: reject incorrect dates #8791

Closed

jacoblipech pushed a commit to jacoblipech/teammates that referenced this pull request May 14, 2018

[TEAMMATES#84] Add support for daylight saving time (TEAMMATES#8687)

864991d

jacoblipech pushed a commit to jacoblipech/teammates that referenced this pull request Feb 14, 2019

[TEAMMATES#84] Add support for daylight saving time (TEAMMATES#8687)

26b7f72

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for daylight saving time #84 #8687

Add support for daylight saving time #84 #8687

whipermr5 commented Mar 20, 2018 •

edited

Loading

wkurniawan07 commented Mar 20, 2018 •

edited

Loading

whipermr5 commented Mar 22, 2018

whipermr5 commented Mar 22, 2018

tran-tien-dat Mar 22, 2018

tran-tien-dat Mar 22, 2018

whipermr5 Mar 22, 2018

tran-tien-dat Mar 22, 2018

tran-tien-dat Mar 22, 2018

whipermr5 Mar 22, 2018

tran-tien-dat Mar 22, 2018 •

edited

Loading

whipermr5 Mar 22, 2018

tran-tien-dat Mar 22, 2018

whipermr5 Mar 22, 2018

tran-tien-dat Mar 22, 2018

tran-tien-dat Mar 22, 2018 •

edited

Loading

tran-tien-dat Mar 22, 2018 •

edited

Loading

darrenwee left a comment •

edited

Loading

darrenwee Mar 23, 2018

darrenwee Mar 23, 2018

darrenwee Mar 23, 2018

darrenwee Mar 23, 2018

tran-tien-dat Mar 23, 2018

whipermr5 Mar 23, 2018

tran-tien-dat Mar 23, 2018

whipermr5 commented Mar 23, 2018 •

edited

Loading

sukanta-27 Apr 2, 2018

whipermr5 Apr 2, 2018

		</select>
		<input type="button" class="btn btn-primary" id="auto-detect-time-zone" value="Auto-Detect Time Zone">

Add support for daylight saving time #84 #8687

Add support for daylight saving time #84 #8687

Conversation

whipermr5 commented Mar 20, 2018 • edited Loading

wkurniawan07 commented Mar 20, 2018 • edited Loading

whipermr5 commented Mar 22, 2018

whipermr5 commented Mar 22, 2018

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 Mar 22, 2018 • 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

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

darrenwee left a comment • 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

whipermr5 commented Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whipermr5 commented Mar 20, 2018 •

edited

Loading

wkurniawan07 commented Mar 20, 2018 •

edited

Loading

tran-tien-dat Mar 22, 2018 •

edited

Loading

tran-tien-dat Mar 22, 2018 •

edited

Loading

tran-tien-dat Mar 22, 2018 •

edited

Loading

darrenwee left a comment •

edited

Loading

whipermr5 commented Mar 23, 2018 •

edited

Loading