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.

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

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

Closed
whipermr5 opened this issue Mar 14, 2018 · 8 comments
Closed
Assignees
Labels
a-Testing Testing-related traits such as efficiency, robustness, coverage p.Low Very little impact; unlikely to do in the near future s.OnHold The issue/PR's validity has been put on hold pending some other event
Milestone

Comments

@whipermr5
Copy link
Member

whipermr5 commented Mar 14, 2018

HtmlHelper requires the time zone to correctly replace now timestamps with datetime.now.

The HTML time zone detection introduced in #7616 is unreliable as it depends on the pattern UTC+xxxx showing up in HTML for it to work. However, pages like StudentHomePage do not contain the time zone in HTML at all, while others like InstructorSearchPage contain multiple time zones. This has resulted in peculiarities in StudentHomePageUiTest - see #8629 (comment). Let's try to come up with a solution that works for all cases (perhaps include the time zone as a data attribute in HTML?). It is now completely broken as the offset is no longer displayed in favour of the time zone short name since #84.

Instead of doing time zone detection, let's wrap all displays of timestamps in a tag with a data attribute containing the corresponding ISO8601 UTC formatted instant. HtmlHelper should then ignore the text contained inside the tag, and leave only the data attribute for comparison. We can then detect now timestamps easily as they would be in a standardised format, unaffected by time zone.


Update: Work-around has been introduced in #8640. Need to find a more elegant way to do this instead.

@whipermr5 whipermr5 added the a-Testing Testing-related traits such as efficiency, robustness, coverage label Mar 14, 2018
@tran-tien-dat
Copy link
Contributor

tran-tien-dat commented Mar 14, 2018

including the time zone as a data attribute in HTML will require more complicated RegEx to detect. The RegEx will have to recover all kinds of HTML elements where the time data can be a children of.

Date now = new Date();
SimpleDateFormat sdf = new SimpleDateFormat("EEE, dd MMM yyyy, ");
// get session's time zone from content.
// this method is not applicable for pages with multiple time zones like InstructorSearchPage
sdf.setTimeZone(getTimeZone(content));
String dateTimeNow = sdf.format(now);
SimpleDateFormat sdfForIso8601 = new SimpleDateFormat("yyyy-MM-dd'T'");
String dateTimeNowInIso8601 = sdfForIso8601.format(now);
SimpleDateFormat sdfForCoursesPage = new SimpleDateFormat("d MMM yyyy");
String dateTimeNowInCoursesPageFormat = sdfForCoursesPage.format(now);

// date/time now e.g [Thu, 07 May 2015, 07:52 PM]
.replaceAll(dateTimeNow + REGEX_DISPLAY_TIME, "\\${datetime\\.now}")
.replaceAll(dateTimeNowInIso8601 + REGEX_DISPLAY_TIME_ISO_8601_UTC, "\\${datetime\\.now\\.iso8601utc}")
.replaceAll(dateTimeNowInCoursesPageFormat, "\\${datetime\\.now\\.courses}")

The time zone detection is basically to figure out the correct string for the current time, and then replace it with a constant string like datetime.now. So how about we change the tests so that they pass a list of strings to be replaced by datetime.now etc. instead? Of course, this will require more work but I think it's more future-proof.

@whipermr5
Copy link
Member Author

whipermr5 commented Mar 29, 2018

I feel a better solution would be to ensure that every time we display a timestamp, we also include its corresponding ISO8601-formatted instant in an enclosing data attribute. This way, we can just ignore the displayed timestamp, and detect and replace the data attribute instead. With #8677, we are already moving towards that.

@whipermr5 whipermr5 added the p.High Significant impact; would like to do in the next few releases label Apr 1, 2018
@whipermr5
Copy link
Member Author

This is causing builds to fail between 12am - 8am SGT daily after #84 was merged 😅

@tran-tien-dat
Copy link
Contributor

I know this may sound a bit radical, but I think it's worth mentioning, at least as a reference of how things should be done if we are writing from scratch. We can move displaying of time to the client totally. So the backend will only produce htmls with data attributes which contain Instant.toEpochMillis() as well as the required timezone. The client-side javascript will use that to insert the right formatted time string.

This has the benefits of simplifying server-side testing, saving server-side computation. The cost is that we have to translate the formatting functions of TimeHelper.java to javascript, and testing will be moved to javascript tests.

If we push the idea even further, we can move all timezone handling to the client side. So the request will just contain the EpochMillis calculated by client-side javascript from the form content. This would mean we can get rid of most of TimeHelper.java on the server side.

@damithc
Copy link
Contributor

damithc commented Apr 6, 2018

Just mentioning in case relevant: There are online global courses where students come from different countries. In those cases, it is better to show times based on the base location of the course, to match with announcements of deadlines by instructors. Trying to show times based on each student's location may lead to confusions. This is the reason why we detect timezone at course/session creation and use it for all subsequent purposes.

@xpdavid
Copy link
Contributor

xpdavid commented Sep 14, 2018

Decrease priority now as frontend migration is underway 🎉 https://github.com/TEAMMATES/teammates/projects/4 . After completion, we will achieve what @tran-tien-dat has suggested and there is no need have full page HTML verification.

@xpdavid xpdavid added p.Low Very little impact; unlikely to do in the near future s.OnHold The issue/PR's validity has been put on hold pending some other event and removed p.High Significant impact; would like to do in the next few releases labels Sep 14, 2018
@xpdavid xpdavid changed the title HtmlHelper: fix broken detection of current time HtmlHelper: Use better way to replace now timestamps with datetime.now Sep 14, 2018
@wkurniawan07
Copy link
Member

there is no need have full page HTML verification.

I can't promise this. "Less HTML verification" is definitive, but "No HTML verification" is too strong a conclusion at this point of time.

@wkurniawan07
Copy link
Member

HtmlHelper is decommissioned in V7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-Testing Testing-related traits such as efficiency, robustness, coverage p.Low Very little impact; unlikely to do in the near future s.OnHold The issue/PR's validity has been put on hold pending some other event
Projects
None yet
Development

No branches or pull requests

5 participants