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.

Sign up for GitHub

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

Jump to bottom

Migrate timezone value from feedback sessions to courses #5928 #8759

Merged
merged 21 commits into from
Apr 10, 2018
Merged

Migrate timezone value from feedback sessions to courses #5928 #8759

merged 21 commits into from
Apr 10, 2018

Conversation

whipermr5
Copy link
Member

@whipermr5 whipermr5 commented Apr 6, 2018 •
edited
Loading

Fixes #5928, part of #8448

Outline of Solution

  • Feedback session retains time zone property but follows course time zone
  • Sessions form now shows time zone as a read only field screen shot 2018-04-09 at 1 45 40 am
    apr-09-2018 01-43-36
  • When adding, editing or copying a session, destination course time zone is followed
  • When editing a course, time zone is cascaded to all feedback sessions in the course

Other Changes

  • When displaying the feedback session form for an existing session, round timestamps to the nearest hour (round to 23:59 of the previous day if 00:00 is the nearest hour) so that the appropriate time will be selected in the hour dropdown
    • This is needed as feedback session times may no longer follow the "23:59 or h:00 where h is not 0" format if the course time zone is changed after the feedback session has been set up

@whipermr5 whipermr5 added the s.Ongoing The PR is being worked on by the author(s) label Apr 6, 2018
@whipermr5 whipermr5 force-pushed the 5928-fs-time-zone-migration branch from 04ea575 to cf9d8ee Compare April 6, 2018 10:18
This was referenced Apr 6, 2018
Closed
Closed
@whipermr5 whipermr5 force-pushed the 5928-fs-time-zone-migration branch 5 times, most recently from 1a9dda3 to dd4ce87 Compare April 6, 2018 18:45
@whipermr5 whipermr5 added this to the V6.5.0 milestone Apr 8, 2018
@whipermr5 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 Apr 8, 2018
@whipermr5 whipermr5 force-pushed the 5928-fs-time-zone-migration branch 2 times, most recently from d02415a to f610bf5 Compare April 9, 2018 14:14
wkurniawan07
wkurniawan07 requested changes Apr 10, 2018
View reviewed changes
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor changes.

src/client/java/teammates/client/scripts/DataMigrationForSessions.java Outdated
* - Conversion of all time fields to UTC
* - Conversion of the timeZone field to a ZoneId string following the course time zone; removal of the timeZoneDouble field
* - Population of any missing is*EmailEnabled booleans to default value true
* - Adjustment of the results visible from time TIME_REPRESENTS_NEVER -> TIME_REPRESENTS_LATER
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this <ul> and <li>s

src/main/java/teammates/storage/entity/FeedbackSession.java
double offset;
if (timeZone.equals(String.valueOf(Const.INT_UNINITIALIZED))) {
offset = timeZoneDouble;
Course course = Ref.create(Key.create(Course.class, courseId)).get();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be removed after data migration, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup!

@whipermr5
Copy link
Member Author

@wkurniawan07 I realised that editing the course time zone after a session is set up can lead to arbitrary start/end/visible/publish times that may not end with :00 or :59. Just pushed a commit to ensure that such times are properly handled by the sessions form.

@whipermr5 whipermr5 added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Apr 10, 2018
@whipermr5 whipermr5 requested a review from damithc April 10, 2018 14:56
@whipermr5 whipermr5 added the e.2 label Apr 10, 2018
@whipermr5
Update time zone display in sessions form
d058131 
No longer an input but a static field
Similar to course name, update time zone display dynamically upon course selection change
@whipermr5
Adjust tooltips
3c71950
@whipermr5
Remove fixed offset warning modal
7f99a0f
@whipermr5
Update InstructorFeedbackEditPageUiTest
25be323 
Add new course and session for DST test case
@whipermr5
Add editCourse method to BackDoor
934ffa2
@whipermr5
Update InstructorFeedbackSessionsPageUiTest
51cc670
@whipermr5
Update remaining test HTML files
f47fd71
@whipermr5
Test for time zone cascade in InstructorCourseEditSaveActionTest
beb9500
@whipermr5
Test that session time zone follows destination course time zone in c…
711f8df 
…opy action tests

Edited time zone for two courses to facilitate testing
Updated dependent test HTML file
@whipermr5
Adjust tooltip
186277c
@whipermr5
Prettify javadoc
4b2093a
@whipermr5
Remove unnecessary suppression
cdd781f
@whipermr5
Sessions form: handle display of arbitrary times that do not conform …
78d5911 
…with the "23:59 or minute is 0 and hour is not 0" contract

These times might now validly be set if the course time zone is edited after sessions have already been set up
E.g. a session with end time of 23:59 in an Asia/Kathmandu course would have an end time of 18:14 if the course time zone is edited to UTC
Such an end time would be selected as 18:00 in the sessions form dropdown (though it would still properly display as 18:14 everywhere else)
@whipermr5
Early exit if time is already in the correct format
e6cb323
@whipermr5 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 10, 2018
@whipermr5
Fix broken isMigrationNeeded detection
b8c3097 
Previously, it would always return false
@whipermr5 whipermr5 removed the request for review from tran-tien-dat April 10, 2018 18:01
@whipermr5 whipermr5 merged commit e15471b into TEAMMATES:master Apr 10, 2018
@whipermr5 whipermr5 mentioned this pull request Apr 10, 2018
Closed
@whipermr5 whipermr5 deleted the 5928-fs-time-zone-migration branch April 10, 2018 18:25
@whipermr5 whipermr5 added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Apr 14, 2018
@whipermr5 whipermr5 mentioned this pull request Apr 20, 2018
Closed
jacoblipech pushed a commit to jacoblipech/teammates that referenced this pull request May 14, 2018
@whipermr5
[TEAMMATES#5928] Migrate timezone value from feedback sessions to cou…
f45cb68 
…rses (TEAMMATES#8759)
@xpdavid xpdavid mentioned this pull request Aug 28, 2018
Closed
5 tasks
@tran-tien-dat tran-tien-dat mentioned this pull request Aug 30, 2018
Merged
jacoblipech pushed a commit to jacoblipech/teammates that referenced this pull request Feb 14, 2019
@whipermr5 @jacoblipech
[TEAMMATES#5928] Migrate timezone value from feedback sessions to cou…
f2dedfc 
…rses (TEAMMATES#8759)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wkurniawan07 wkurniawan07 wkurniawan07 approved these changes

@damithc damithc damithc approved these changes

Assignees

@wkurniawan07 wkurniawan07

Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V6.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@whipermr5 @damithc @wkurniawan07