Instructor: home page: fix sort by date #7919 #8677

nidhi98gupta · 2018-03-18T20:26:11Z

Continuing the work done by @HirdayGupta according to the #7966 (comment).

Outline of Solution:
This PR will add ISO8601 date stamps to all the date elements of TEAMMATES. (Hopefully I have covered all)
I have added the data-date-stamp to the tables on the instructorHomePage and changed the extractor to dateStampExtractor from tooltipExtractor, which fixes the Sort By Date bug.
Also added the end-time date-stamp to the studentHomePage and ISO8601 datestamp to the start-time in feedbackSessionDetailsPanel tag.

nidhi98gupta · 2018-03-20T06:01:46Z

Adding date stamp has made student home page an unpredictable html resulting in build failure. I will try solving it using #8332 (comment).

nidhi98gupta · 2018-03-20T17:33:14Z

Ready for review.

nidhi98gupta · 2018-03-21T09:35:32Z

@joanneong Ready for review.

tran-tien-dat · 2018-03-21T14:53:23Z

@nidhi98gupta Can you do an interactive rebase onto master and delete the merge commits so that the PR looks cleaner and easier to review?

tran-tien-dat · 2018-03-21T14:55:57Z

Oh nevermind, there are no merge conflicts so the changes in the merge conflict is not reflected in the PR.

nidhi98gupta · 2018-03-27T12:37:50Z

@darrenwee This is ready for review. Build is failing due to Bad Gateway error. I haven't added any new commits since last build success. Please review.

darrenwee · 2018-03-27T15:50:21Z

Hi @nidhi98gupta! The build failed because the dependency repository went down. I've triggered a rebuild and I'll review within the next few days! You do not have to keep updating the branch to keep up with master!

darrenwee · 2018-03-28T10:29:56Z

Can you perform the following git commands to clean up the branch history? I'm puzzled by the build failure because it doesn't seem like there's anything that changed between the first failing build and the last passing build, and I don't get the same failures locally.

git remote add upstream https://github.com/TEAMMATES/teammates.git # if you already have this, you can skip this step
git fetch upstream
git checkout 7919-sort-by-date
git rebase upstream/master
git push -f

nidhi98gupta · 2018-03-28T19:24:49Z

@darrenwee I rebased the changes and build has passed! Kindly review. :)

whipermr5 · 2018-03-29T03:19:23Z

👍👍 This would really help #8640 too (no need for time zone detection anymore if we know the exact instant each time stamp is referring to)!

darrenwee · 2018-03-29T05:37:09Z

Hey @nidhi98gupta, the injected time stamps look okay to me, but the feature doesn't seem to be working. Can you take a look and make sure the data-date-stamp is correctly extracted, parsed and sorted on?

~~The order of the April 2, 11:00PM end date and the April 2, 11:59PM one is not correct in ascending/descending sort.~~

Sorry, I did not see the year field 😅 I'll test it a bit more.

darrenwee

Looks pretty good! I have requested some naming changes for better documentation, but everything looks pretty solid.

There are two more places that you can add the sort by date functionality into.

the student's home page (at /page/studentHomePage) - the Deadline column doesn't have any sorting functionality.
the admin's sessions page (at /admin/adminSessionsPage) - the sort here doesn't work correctly. You'll need to inject the time stamp and add the extractor to the header cell.

Once you have done that, remember to rebase onto upstream/master and ping me again 😃

darrenwee · 2018-03-29T08:21:13Z

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

@@ -610,6 +610,10 @@ public String getEndTimeInIso8601Format() {
        return TimeHelper.formatInstantToIso8601Utc(endTime);
    }

+    public String getStartTimeInIso8601Format() {


Rename this method to getStartTimeInIso8601UtcFormat and do the same for the EndTime equivalent. This is because ISO 8601 timestamps can have a UTC-offset, so it's best to disambiguate in the getter's name.

darrenwee · 2018-03-29T08:22:43Z

src/main/java/teammates/ui/template/InstructorHomeFeedbackSessionRow.java

@@ -3,19 +3,24 @@
 public class InstructorHomeFeedbackSessionRow extends HomeFeedbackSessionRow {
    private String startTime;
    private String startTimeToolTip;
+    private String startTimeDateStamp;


Rename this field to something more self-explanatory, like startTimeIso8601Utc. Since the type is of String and not Instant, it cannot be inferred right away that the time stamp here is on the universal timeline.

darrenwee · 2018-03-29T08:22:57Z

src/main/java/teammates/ui/template/InstructorHomeFeedbackSessionRow.java

    private String endTime;
    private String endTimeToolTip;
+    private String endTimeDateStamp;


Same as above.

darrenwee · 2018-03-29T08:23:57Z

src/main/java/teammates/ui/template/InstructorHomeFeedbackSessionRow.java

    private String href;
    private InstructorFeedbackSessionActions actions;

    public InstructorHomeFeedbackSessionRow(String name, String submissionsTooltip, String publishedTooltip,
-            String submissionStatus, String publishedStatus, String startTime, String startTimeToolTip,
-            String endTime, String endTimeToolTip, String href, InstructorFeedbackSessionActions actions) {
+            String submissionStatus, String publishedStatus, String startTime, String startTimeDateStamp,


Same as above.

darrenwee · 2018-03-29T08:25:04Z

src/main/java/teammates/ui/template/StudentHomeFeedbackSessionRow.java

@@ -2,14 +2,16 @@

 public class StudentHomeFeedbackSessionRow extends HomeFeedbackSessionRow {
    private String endTime;
+    private String endTimeDateStamp;


Rename this field to something more self-explanatory, like endTimeIso8601Utc. Since the type is of String and not Instant, it cannot be inferred right away that the time stamp here is on the universal timeline.

darrenwee · 2018-03-29T08:25:29Z

src/main/java/teammates/ui/template/StudentHomeFeedbackSessionRow.java

    private StudentFeedbackSessionActions actions;
    private int index;

    public StudentHomeFeedbackSessionRow(String name, String submissionsTooltip, String publishedTooltip,
-            String submissionStatus, String publishedStatus, String endTime, StudentFeedbackSessionActions actions,
-            int index) {
+            String submissionStatus, String publishedStatus, String endTime, String endTimeDateStamp,


Same as above.

darrenwee · 2018-03-29T08:26:27Z

src/main/webapp/WEB-INF/tags/instructor/home/courseTable.tag

@@ -44,10 +44,10 @@
          <td>
            ${sessionRow.name}
          </td>
-          <td class="text-nowrap">
+          <td class="text-nowrap" data-date-stamp="${sessionRow.startTimeDateStamp}">


All the templates need to be updated to reflect the change in variable names from above.

whipermr5 · 2018-03-30T08:44:34Z

src/main/webapp/WEB-INF/tags/shared/feedbackSubmissionEdit/feedbackSessionDetailsPanel.tag

@@ -27,7 +27,7 @@
        <div class="form-group">
          <label class="col-sm-2 control-label">Opening time:</label>
          <div class="col-sm-10">
-            <p class="form-control-static">${feedbackSession.startTimeString}</p>
+            <p class="form-control-static" data-start-time="${feedbackSession.startTimeInIso8601Format}">${feedbackSession.startTimeString}</p>


Can we standardise the name of the data attribute? It should be the same here as in all the other files.

… studentHomePage

wkurniawan07 · 2018-04-03T04:29:53Z

On that topic of 12:00 NOON, @damithc is there any tangible benefit of doing this? The code is subjected to some non-trivial workarounds (and later on tests) just to support that.

whipermr5 · 2018-04-03T04:44:06Z

src/main/java/teammates/ui/pagedata/StudentHomePageData.java

@@ -71,6 +71,7 @@ private void setCourseTables(List<CourseDetailsBundle> courses,
                    getStudentSubmissionStatusForSession(feedbackSession, hasSubmitted),
                    getStudentPublishedStatusForSession(feedbackSession),
                    TimeHelper.formatTime12H(feedbackSession.getEndTimeLocal()),


This isn't directly related, but since you are improving the student home page by adding the sort by deadline, could you take this chance to also show the time zone info in the deadline display? Simply change formatTime12H here to formatDateTimeForSessions and run godmode. Forgot to do this in #8687 😅

damithc · 2018-04-03T04:51:37Z

On that topic of 12:00 NOON, @damithc is there any tangible benefit of doing this? The code is subjected to some non-trivial workarounds (and later on tests) just to support that.

What are our options? I think we started using NOON to clearly distinguish it from midnight. Apparently, there is no clear agreement about the use of 12 AM and 12 PM https://en.wikipedia.org/wiki/12-hour_clock#Confusion_at_noon_and_midnight Even if there is a definitive answer, some users might not be aware of it and make mistakes. Anyway we can minimize user mistakes but also not bust our backs on coding/testing?

whipermr5 · 2018-04-03T06:36:57Z

The code is subjected to some non-trivial workarounds (and later on tests) just to support that.

Actually once #8640 is properly fixed through the use of data attributes everywhere, display would be pretty much independent of sorting/testing etc. I can only think of three workarounds that exist in the code to support this feature:

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

Lines 282 to 292 in dc85dd0

    
           private static String formatLocalDateTime(LocalDateTime localDateTime, String pattern) { 
        
               if (localDateTime == null || pattern == null) { 
        
                   return ""; 
        
               } 
        
               String processedPattern = pattern; 
        
               if (localDateTime.getHour() == 12 && localDateTime.getMinute() == 0) { 
        
                   processedPattern = pattern.replace("a", "'NOON'"); 
        
               } 
        
               DateTimeFormatter formatter = DateTimeFormatter.ofPattern(processedPattern); 
        
               return localDateTime.format(formatter); 
        
           }

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

Lines 345 to 356 in dc85dd0

    
           private static String formatInstant(Instant instant, ZoneId timeZone, String pattern) { 
        
               if (instant == null || timeZone == null || pattern == null) { 
        
                   return ""; 
        
               } 
        
               ZonedDateTime zonedDateTime = instant.atZone(timeZone); 
        
               String processedPattern = pattern; 
        
               if (zonedDateTime.getHour() == 12 && zonedDateTime.getMinute() == 0) { 
        
                   processedPattern = pattern.replace("a", "'NOON'"); 
        
               } 
        
               DateTimeFormatter formatter = DateTimeFormatter.ofPattern(processedPattern); 
        
               return zonedDateTime.format(formatter); 
        
           }

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

Line 39 in dc85dd0

    
           private static final String REGEX_DISPLAY_TIME = "(0[0-9]|1[0-2]):[0-5][0-9] ([AP]M|NOON)";

Furthermore, the last one will be removed once #8640 is properly fixed. As such, I find the current workarounds acceptable. This issue only arose because display of dates was tied to logic, and we have been moving away from such coupling with the use of data attributes.

darrenwee · 2018-04-03T07:02:32Z

Maybe to get rid of the workarounds in TimeHelper, we could just format the dates as per normal (like dd MM yyy hh:mm a or whatever it is) and insert a badge or label to indicate noon or midnight?

whipermr5

LGTM 👍 (Once the two test cases are updated to fix the build)

whipermr5 · 2018-04-03T10:01:19Z

src/test/java/teammates/test/cases/pagedata/StudentHomePageDataTest.java

@@ -131,7 +131,8 @@ private void testFeedbackSession(int index, HomeFeedbackSessionRow row, Feedback
            String expectedPublishedStatus) {
        StudentHomeFeedbackSessionRow studentRow = (StudentHomeFeedbackSessionRow) row;
        assertEquals(session.getFeedbackSessionName(), studentRow.getName());
-        assertEquals(TimeHelper.formatTime12H(session.getEndTimeLocal()), studentRow.getEndTime());
+        assertEquals(TimeHelper.formatDateTimeForSessions(session.getEndTime(), session.getTimeZone()),
+                     studentRow.getEndTime());


8-space indentation. You may need to change your IDE settings?

IntelliJ indented it in 8 spaces, but then I looked at other assertEquals statements in the codebase just to be sure, it was indented like this only.
Nevermind I'll change it.

👍 The codebase is still in transition towards 8-space indentation; it hasn't been applied everywhere yet.

)

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

joanneong requested a review from darrenwee March 21, 2018 09:42

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

nidhi98gupta closed this Mar 27, 2018

nidhi98gupta reopened this Mar 27, 2018

darrenwee closed this Mar 27, 2018

darrenwee reopened this Mar 27, 2018

darrenwee self-assigned this Mar 27, 2018

nidhi98gupta force-pushed the 7919-sort-by-date branch from fe32fa9 to 64a3a0f Compare March 28, 2018 18:24

whipermr5 mentioned this pull request Mar 29, 2018

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

Closed

darrenwee requested changes Mar 29, 2018

View reviewed changes

whipermr5 requested changes Mar 30, 2018

View reviewed changes

whipermr5 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 Mar 30, 2018

nidhi98gupta added 4 commits March 30, 2018 15:46

Add getStartTimeInIso8601Format to FSAttributes

fc1966a

Add end-time and start-time date-stamp data to instructorHomePage and…

443677c

… studentHomePage

Update HTML files

d277ec4

Update HTML files

719a872

whipermr5 reviewed Apr 3, 2018

View reviewed changes

Modify test data

3a645e4

whipermr5 approved these changes Apr 3, 2018

View reviewed changes

Update tests and html files

cf72c0c

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

fix lint error

c340b7a

whipermr5 reviewed Apr 3, 2018

View reviewed changes

Indent

dbb45cc

darrenwee approved these changes Apr 3, 2018

View reviewed changes

whipermr5 added s.FinalReview The PR is ready for final review e.2 and removed s.Ongoing The PR is being worked on by the author(s) labels Apr 3, 2018

whipermr5 requested a review from damithc April 3, 2018 10:40

whipermr5 changed the title ~~Instructor: Home Page: Sort By Date does not work as expected #7919~~ Instructor: home page: fix sort by date #7919 Apr 3, 2018

Merge branch 'master' into 7919-sort-by-date

8a1d12e

damithc approved these changes Apr 3, 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 Apr 3, 2018

whipermr5 merged commit 2b0e949 into TEAMMATES:master Apr 3, 2018

nidhi98gupta deleted the 7919-sort-by-date branch April 4, 2018 17:20

whipermr5 added the c.Bug Bug/defect report label Apr 14, 2018

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

[TEAMMATES#7919] Instructor: home page: fix sort by date (TEAMMATES#8677

f20fe26

)

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

[TEAMMATES#7919] Instructor: home page: fix sort by date (TEAMMATES#8677

251f7b2

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instructor: home page: fix sort by date #7919 #8677

Instructor: home page: fix sort by date #7919 #8677

nidhi98gupta commented Mar 18, 2018 •

edited

Loading

nidhi98gupta commented Mar 20, 2018

nidhi98gupta commented Mar 20, 2018

nidhi98gupta commented Mar 21, 2018

tran-tien-dat commented Mar 21, 2018 •

edited

Loading

tran-tien-dat commented Mar 21, 2018

nidhi98gupta commented Mar 27, 2018

darrenwee commented Mar 27, 2018 •

edited

Loading

darrenwee commented Mar 28, 2018 •

edited

Loading

nidhi98gupta commented Mar 28, 2018

whipermr5 commented Mar 29, 2018

darrenwee commented Mar 29, 2018 •

edited

Loading

darrenwee left a comment •

edited

Loading

darrenwee Mar 29, 2018

darrenwee Mar 29, 2018

darrenwee Mar 29, 2018

darrenwee Mar 29, 2018

darrenwee Mar 29, 2018

darrenwee Mar 29, 2018

darrenwee Mar 29, 2018

whipermr5 Mar 30, 2018

wkurniawan07 commented Apr 3, 2018

whipermr5 Apr 3, 2018

damithc commented Apr 3, 2018

whipermr5 commented Apr 3, 2018 •

edited

Loading

darrenwee commented Apr 3, 2018

whipermr5 left a comment •

edited

Loading

whipermr5 Apr 3, 2018

nidhi98gupta Apr 3, 2018

whipermr5 Apr 3, 2018

Instructor: home page: fix sort by date #7919 #8677

Instructor: home page: fix sort by date #7919 #8677

Conversation

nidhi98gupta commented Mar 18, 2018 • edited Loading

nidhi98gupta commented Mar 20, 2018

nidhi98gupta commented Mar 20, 2018

nidhi98gupta commented Mar 21, 2018

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

tran-tien-dat commented Mar 21, 2018

nidhi98gupta commented Mar 27, 2018

darrenwee commented Mar 27, 2018 • edited Loading

darrenwee commented Mar 28, 2018 • edited Loading

nidhi98gupta commented Mar 28, 2018

whipermr5 commented Mar 29, 2018

darrenwee commented Mar 29, 2018 • edited Loading

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

Choose a reason for hiding this comment

wkurniawan07 commented Apr 3, 2018

Choose a reason for hiding this comment

damithc commented Apr 3, 2018

whipermr5 commented Apr 3, 2018 • edited Loading

darrenwee commented Apr 3, 2018

whipermr5 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

nidhi98gupta commented Mar 18, 2018 •

edited

Loading

tran-tien-dat commented Mar 21, 2018 •

edited

Loading

darrenwee commented Mar 27, 2018 •

edited

Loading

darrenwee commented Mar 28, 2018 •

edited

Loading

darrenwee commented Mar 29, 2018 •

edited

Loading

darrenwee left a comment •

edited

Loading

whipermr5 commented Apr 3, 2018 •

edited

Loading

whipermr5 left a comment •

edited

Loading