launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #17096
Re: [Merge] lp:~cjohnston/launchpad/ic-approx-time into lp:launchpad
Review: Needs Fixing
Diff comments:
> === added file 'lib/lp/app/javascript/date.js'
> --- lib/lp/app/javascript/date.js 1970-01-01 00:00:00 +0000
> +++ lib/lp/app/javascript/date.js 2014-07-08 00:33:34 +0000
> @@ -0,0 +1,48 @@
> +/* Copyright 2014 Canonical Ltd. This software is licensed under the
> + * GNU Affero General Public License version 3 (see the file LICENSE). */
> +
> +YUI.add('lp.app.date', function(Y) {
> +
> +var namespace = Y.namespace('lp.app.date');
> +
> +namespace.parse_date = function(str) {
> + re = /^(\d{4})-(\d{2})-(\d{2})T(\d{2}):(\d{2}):(\d{2})(?:\.(\d+))?(Z|\+00:00)$/
> + bits = re.exec(str).slice(1, 8).map(Number)
> + bits[1] -= 1
> + return new Date(Date.UTC.apply(null, bits))
> +};
This defines two global variables, and could use comments describing what it parses and what that bits[1] -= 1 line does.
> +
> +namespace.approximatedate = function(current_date) {
current_date is not the current date.
> + // Display approximate time an event happened when it happened less than 1
> + // day ago.
> + var now = (new Date).valueOf();
> + if (typeof current_date == "string") {
> + var timedelta = now - namespace.parse_date(current_date);
> + } else {
> + var timedelta = now - current_date
> + }
That type check should use ===, and the whole timedelta duplication thing is a bit weird. Consider having approximatedate take just a Date object rather than a Date object or a string.
> + var days = timedelta / 86400000
> + var hours = timedelta / 3600000
> + var minutes = timedelta / 60000
> + var amount = 0
> + var unit = ""
> + if (days > 1) {
> + return 'on ' + Y.Date.format(
> + new Date(current_date), {format: '%Y-%m-%d'});
> + } else {
> + if (hours >= 1) {
> + amount = hours
> + unit = "hour"
> + } else if (minutes >= 1) {
> + amount = minutes
> + unit = "minute"
> + } else {
> + return "a moment ago"
> + }
> + if (Math.floor(amount) > 1) {
> + unit = unit + 's'
> + }
> + return Math.floor(amount) + ' ' + unit + ' ago'
> + }
> +};
> +}, "0.1", {'requires': ['datatype-date']});
>
> === added file 'lib/lp/app/javascript/tests/test_date.html'
> --- lib/lp/app/javascript/tests/test_date.html 1970-01-01 00:00:00 +0000
> +++ lib/lp/app/javascript/tests/test_date.html 2014-07-08 00:33:34 +0000
> @@ -0,0 +1,37 @@
> +<!DOCTYPE html>
> +<!--
> +Copyright 2014 Canonical Ltd. This software is licensed under the
> +GNU Affero General Public License version 3 (see the file LICENSE).
> +-->
> +<html>
> + <head>
> + <title>Test lp-names</title>
> + <!-- YUI and test setup -->
> + <script type="text/javascript"
> + src="../../../../../build/js/yui/yui/yui.js">
> + </script>
> + <link rel="stylesheet"
> + href="../../../../../build/js/yui/console/assets/console-core.css" />
> + <link rel="stylesheet"
> + href="../../../../../build/js/yui/test-console/assets/skins/sam/test-console.css" />
> + <link rel="stylesheet"
> + href="../../../../../build/js/yui/test/assets/skins/sam/test.css" />
> +
> + <script type="text/javascript"
> + src="../../../../../build/js/lp/app/testing/testrunner.js"></script>
> + <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
> +
> + <!-- The module under test. -->
> + <script type="text/javascript" src="../date.js"></script>
> +
> + <!-- The test suite. -->
> + <script type="text/javascript" src="test_date.js"></script>
> +
> + </head>
> + <body class="yui3-skin-sam">
> + <ul id="suites">
> + <li>date.test</li>
> + </ul>
> +
> + </body>
> +</html>
>
> === added file 'lib/lp/app/javascript/tests/test_date.js'
> --- lib/lp/app/javascript/tests/test_date.js 1970-01-01 00:00:00 +0000
> +++ lib/lp/app/javascript/tests/test_date.js 2014-07-08 00:33:34 +0000
> @@ -0,0 +1,39 @@
> +YUI.add('date.test', function (Y) {
> +
> + var tests = Y.namespace('date.test');
> + tests.suite = new Y.Test.Suite("date tests");
> +
> + var now = (new Date).valueOf()
> + tests.suite.add(new Y.Test.Case({
> + name: 'test_approximatedate',
> +
> + test_return_moment_ago: function () {
> + Y.Assert.areEqual(
> + 'a moment ago',
> + Y.lp.app.date.approximatedate(new Date(now - 150)));
> + },
> +
> + test_return_minute_ago: function () {
> + Y.Assert.areEqual(
> + '1 minute ago',
> + Y.lp.app.date.approximatedate(new Date(now - 65000)));
> + },
> +
> + test_return_hours_ago: function () {
> + Y.Assert.areEqual(
> + '3 hours ago',
> + Y.lp.app.date.approximatedate(new Date(now - 12600000)));
> + },
> + // XXX cjohnston 20140707: test disabled due to ES4 lack of
> + // ISO8601 support. Although the vast majority of production
> + // clients run ES5.
> + // test_return_days_ago: function () {
> + // Y.Assert.areEqual(
> + // 'on 2012-08-12', Y.lp.app.date.approximatedate(
> + // '2012-08-12T10:00:00.00001+00:00'));
> + // },
Does this still need to be disabled?
> + }));
> +
> +}, '0.1', {
> + requires: ['lp.testing.runner', 'test', 'lp', 'lp.app.date']
> +});
>
> === modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
> --- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-05-29 16:30:42 +0000
> +++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-07-08 00:33:34 +0000
> @@ -184,9 +184,8 @@
> // XXX cprov 20140226: the date should be formatted according to
> // the user locale/timezone similar to fmt:displaydate.
> // We should leverage Y.Intl features.
> - var fmt_date = 'on ' + Y.Date.format(
> - new Date(comment.date), {format: '%Y-%m-%d'});
> - headerspan.one('span').set('text', fmt_date);
> + var date = Y.lp.app.date.approximatedate(comment.date);
> + headerspan.one('span').set('text', date);
> newrow.one('.boardCommentDetails').append(headerspan);
> newrow.one('.boardCommentBody div').append('<span></span>');
> newrow.one('.boardCommentBody').one('span').set('text', comment.text);
> @@ -701,4 +700,4 @@
> namespace.DiffNav = DiffNav;
>
> }, '0.1', {requires: ['datatype-date', 'event', 'io', 'node', 'widget',
> - 'lp.client', 'lp.ui.editor']});
> + 'lp.client', 'lp.ui.editor', 'lp.app.date']});
>
> === modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html'
> --- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html 2014-05-19 21:56:02 +0000
> +++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html 2014-07-08 00:33:34 +0000
> @@ -32,6 +32,8 @@
> <script type="text/javascript"
> src="../../../../../build/js/lp/app/client.js"></script>
> <script type="text/javascript"
> + src="../../../../../build/js/lp/app/date.js"></script>
> + <script type="text/javascript"
> src="../../../../../build/js/lp/app/testing/mockio.js"></script>
> <script type="text/javascript"
> src="../../../../../build/js/lp/app/ui/ui.js"></script>
>
> === modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
> --- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-05-26 12:44:57 +0000
> +++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-07-08 00:33:34 +0000
> @@ -70,25 +70,31 @@
> "ws.op=getInlineComments&previewdiff_id=1",
> mockio.requests[0].config.data);
> mockio.last_request = mockio.requests[0];
> + var now = (new Date).valueOf()
> published_comments = [
> {'line_number': '2',
> 'person': person_obj,
> 'text': 'This is preloaded.',
> - 'date': '2012-08-12T10:00:00.00001+00:00'}
> + 'date': '2012-08-12T10:00:00.00001+00:00'},
> + {'line_number': '3',
> + 'person': person_obj,
> + 'text': 'This is great.',
> + 'date': (new Date(now - 12600000)),
> + },
> ];
> mockio.success({
> responseText: Y.JSON.stringify(published_comments),
> responseHeaders: {'Content-Type': 'application/json'}});
>
> // Published comment is displayed.
> - var comments = Y.one('#diff-line-2').next().one('div');
> + var first_comments = Y.one('#diff-line-2').next().one('div');
> // XXX cprov 20140226: test disabled due to ES4 lack of
> // ISO8601 support. Although the vast majority of production
> // clients run ES5.
> //Y.Assert.areEqual(
> // 'Foo Bar (name16) wrote on 2012-08-12',
> // header.get('text'));
Does this still need to be disabled?
> - var first = comments.one('div:first-child')
> + var first = first_comments.one('div:first-child')
> Y.Assert.areEqual(
> 'This is preloaded.',
> first.one('.boardCommentBody').get('text'));
> @@ -103,12 +109,17 @@
> 'Boing!', second.one('.boardCommentBody').get('text'));
>
> // Draft comment for line 3 is displayed.
> - var third = Y.one('#diff-line-3').next();
> + var second_comments = Y.one('#diff-line-3').next().one('div');
> + var third = second_comments.one('div:first-child')
> Y.Assert.areEqual(
> - 'Unsaved comment',
> + 'Foo Bar (name16) wrote 3 hours ago:',
> third.one('.boardCommentDetails').get('text'));
> - Y.Assert.areEqual(
> - 'Zoing!', third.one('.boardCommentBody').get('text'));
> + var fourth = third.next()
> + Y.Assert.areEqual(
> + 'Unsaved comment',
> + fourth.one('.boardCommentDetails').get('text'));
> + Y.Assert.areEqual(
> + 'Zoing!', fourth.one('.boardCommentBody').get('text'));
> },
>
> test_draft_handler: function() {
>
--
https://code.launchpad.net/~cjohnston/launchpad/ic-approx-time/+merge/225420
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
References
